-
Notifications
You must be signed in to change notification settings - Fork 29
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
Allow beetmover to receive optional exclude patterns #1100
base: master
Are you sure you want to change the base?
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.
Thank you for doing this! It will enable some cleanup for Firefox as well.
I'm assuming that it's tasks like https://firefox-ci-tc.services.mozilla.com/tasks/TI0KzPifSVuVLjPBuydzyw that we want to exclude from uploading the .deb
packages - please let me know if that's incorrect.
What's here looks good. https://github.com/mozilla-releng/scriptworker-scripts/blob/master/beetmoverscript/src/beetmoverscript/data/release_beetmover_task_schema.json will need to updated for this as well.
Once that change is done, we can push it to the dev
beetmoverscript workers to test it against https://github.com/mozilla-releng/staging-mozilla-vpn-client. (You'll need to suffix the worker-type with -dev
to do that, as well.)
@@ -203,6 +203,7 @@ async def push_to_releases_gcs(context): | |||
|
|||
# Weed out RELEASE_EXCLUDE matches, but allow partners specified in the payload | |||
push_partners = context.task["payload"].get("partners", []) | |||
exclude = context.task["payload"].get("exclude", []) + list(RELEASE_EXCLUDE) |
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 isn't a problem now, but I will note that this will subject VPN pushes to the existing RELEASE_EXCLUDE
patterns, which could theoretically be a problem in the future. Hopefully we'll have removed this hardcode by the time any such thing comes up.
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.
From my understanding of the beetmover scripts, that was always the case and I felt that this approach was the best way to minimize the potential for unintended changes in behaviour. Once this PR lands we can probably work towards removing RELEASE_EXCLUDE
by migrating the patterns into their associated tasks.
The other way we could have done this would be to use RELEASE_EXCLUDE
as the default value for existing tasks that rely on RELEASE_EXCLUDE
to omit files, for example:
exclude = context.task["payload"].get("exclude", list(RELEASE_EXCLUDE))
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.
You're right - this is already the case. Sorry for the confusion. I think what you have here is fine, but the above suggestion would be fine as well, if you want to go that route.
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.
Sorry for the delay here.
This patch attempts to add the ability for tasks to specify artifacts which should be excluded from beetmover. This has previously been done by updating the
RELEASE_EXCLUDE
inconstants.py
but that is a bit of a fragile way of making changes, and it would be handy if the tasks could provide this exclusion information in the payload.With this change, a task can provide an optional
exclude
field in the payload, which contains an array of regex patterns to exclude from the beetmover action. For example:As perhaps a bit of an open question, should this be a literal path instead of a regex pattern? I went with regex simply because that's what is currently done with
RELEASE_EXCLUDE