dont build a package with staged changes#7214
Conversation
|
@islemaster @wjordan Interested in feedback on this PR, and insight as to how we might solve bug 2. One option might be to make it so that APPS_TASK is keyed off of the same hash that we use to create our package. However, I'm not sure how easy that is to do given that build_task wants to look at timestamps of files. A more hacky solution might be to have a file that contains our hash. We always calculate our hash, and if it differs from the one in our hash file, we write the new one to that file. We then key APPS_TASK off of the hash file (which should be changing if and only if our hash changed). |
|
What I think might be a better option...always run the apps and code-studio tasks. In cases where there weren't any commit changes, our packager will generate a hash, realize it has the package already and exit, so we're not doing excessive work in cases where there were no changes. |
|
I've done this with my latest commit. BLOCKLY_CORE_TASK is still a build_task that keys off of relevant files and their timestamps. At some point, it probably makes sense to make this be a package more similar to apps/code-studio. Apps and code studio tasks always run, but should exit early in the cases where nothing actually changed. This means that their change detection is consistent across all callers (i.e. here and when we call ensure package in our Rakefile) |
|
I like using the same hash check in both places, that feels like a step forward. So does failing the build when there are unstaged changes that would go into the built package, though I wonder how much of a headache that will be moving forward. |
|
@islemaster @wjordan i'm going to assume this is good and merge this morning unless i hear any reason not to :) |
|
👍 |
dont build a package with staged changes
When determining whether or not to run our APPS_TASK during CI, we look at the timestamps of the files in
apps/. This got us into trouble in the following scenario(1) Make some i18n changes on the staging server. These are local changes that haven't affected the commit history
BUG 1: During this period, we're generating apps packages that have these local changes, but that isn't being reflected in our package hash, since it looks only at commits
(2) Commit local changes on staging server, and push them to origin/staging
(3) CI kicks in due to changes to origin/staging. It decides whether or not it should run APPS_TASK based on timestamps of files in
apps/.BUG 2: Because the latest commit contains files that were changed locally, their timestamps are from before the commit, and we don't actually run APPS_TASK to generate a new package.
As I described this, I'm realizing this PR currently catches/prevents bug 1, but I think can still hit bug 2.
Note: These were both problems before my S3 stuff as well. Local, uncommitted changes would have ended up in our committed apps-package. Committing local changes in apps would not trigger an apps build (because of outdated timestamps).