-
Notifications
You must be signed in to change notification settings - Fork 6
Adding rebase patches step to update-automation workflow #36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be main instead of rebase-patches?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. The rebase-patches was for testing purposes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's also a 4th one here - code-editor-web-embedded-with-terminal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't add it because I didn't see it in build-targets.yaml should I also add it there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, we can add it over there too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we have created this as a separate job, independent of the previous update-automation job. From GitHub docs
by default, jobs have no dependencies and run in parallel
If this job runs in parallel with the update-automation job, then failure handling won't work as expected for us. We should move this step as part of the update-automation job itself, right after the rebasing for all targets step. Example here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The needs: attribute of the handle-failures job specifies that it is run after update-automation is finished (they don't run in parallel), and the if attribute activates the job only if update-automation ended in failure. I created it as a separate job because there are multiple points in the update-automation workflow where metrics would need to be sent to CW in case of failure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense! But we will need to publish a failure metric in case any of the step in the update-automation job fails, and not just if the rebasing step fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also use version here for all configuration files, just to keep it consistent with the internal ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will add.
scripts/prepare-src.sh
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of using boolean flags here to determine if rebasing should be executed or not, we should use different functions here. Extract all the common functionality to functions and then call them one by one. For example:
apply_overrides() { echo "Applying overrides" rsync -a "${PRESENT_WORKING_DIR}/overrides/" "${PATCHED_SRC_DIR}" rsync -a "${PRESENT_WORKING_DIR}/$overrides_path/" "${PATCHED_SRC_DIR}" echo "Applying package-lock overrides" rsync -a "${PRESENT_WORKING_DIR}/package-lock-overrides/sagemaker.series/" "${PATCHED_SRC_DIR}" rsync -a "${PRESENT_WORKING_DIR}/$package_lock_path/" "${PATCHED_SRC_DIR}" } prepare_patch_directory() { echo "Cleaning build src dir" rm -rf "${PATCHED_SRC_DIR}" # Copy third party source echo "Copying third party source to the patch directory" rsync -a "${PRESENT_WORKING_DIR}/third-party-src/" "${PATCHED_SRC_DIR}" } # Any more common functionality which can be extracted to a function
And then write different methods for preparing source and rebasing:
prepare-src() { prepare_patch_directory # Apply patches apply_overrides } rebase() { prepare_patch_directory check_unsaved_changes rebase apply_overrides }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will refactor according to this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this command is going to fail for code-editor-server and code-editor-web-embedded, because there is no folders package-lock-overrides/web-server.series or package-lock-overrides/web-server.series. We need to add empty folders
Description of changes:
scripts/prepare-src.sh. Rebasing is run by adding the--rebaseflag when running the script.update-automation.yamlfor handling failures and sending metrics to CW (the job is activated when the update-automation job fails). The logic for sending metrics to CW will be implemented in a following PR.prepare-src.shscript to be runnable for different targets..jsonconfiguration files for each target.quilt refreshto save his changes to the patch) and if there are any it exits and informs the user.quilt push -f -mand if the command fails it captures the missing files or conflicts from the comand output and generates instructions for the user.Testing:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.