Automation Rules: support external versions #7664
Conversation
stsewd
force-pushed
the
automation-rules-pr
branch
8 times, most recently
from
c4c6dc9 to
1aaa918
Compare
| for regex in GITHUB_REGEXS: | ||
| match = regex.search(url) | ||
| for pattern in GITHUB_REGEXS: | ||
| match = pattern.search(url) |
There was a problem hiding this comment.
the linter doesn't like having these named regex.
- Use a namedtuple to pass more data about the external version around - Automation rules can now receive extra kwargs to be passed down to the match and action functions - Automation rules now return a tuple, the first element indicate if rule matched and the second is the return value from the action, this was done, so we can know if the build was triggered or not for external versions. - Moved the actions to the base class, since it doesn't look like we are going to have a different set of actions - If the user doesn't have automation rules for external versions we just build everything as before, but if the user has at least one, we use the rules to trigger the build. Ref #7653
stsewd
force-pushed
the
automation-rules-pr
branch
from
1aaa918 to
de246b9
Compare
stsewd
removed
the
PR: work in progress
|
This is ready, but maybe wait for the new templates? Or maybe not as this is modifying just the js code. |
There was a problem hiding this comment.
This looks like a good refactor. I didn't do a full review of it, but wanted to throw a couple comments in here.
This feels really valuable, especially the bits around limiting PR building based on regex. I think reducing our PR build load is going to be important going forward, so having more options for this would be really useful. Another area I've thought about having the option to build PR's is on the list of modified files. That's a larger change though for sure :)
| - Base branch: ``^main$`` | ||
| - Action: ``Build version`` | ||
|
|
||
| Build all pull request where the source branch has the ``docs/`` prefix |
There was a problem hiding this comment.
This is super useful, and we should probably be promoting this more.
| @@ -133,6 +138,7 @@ def __init__(self, *args, **kwargs): | |||
| (None, '-' * 9), | |||
| (BRANCH, BRANCH_TEXT), | |||
| (TAG, TAG_TEXT), | |||
| (EXTERNAL, EXTERNAL_TEXT), | |||
There was a problem hiding this comment.
Pretty sure we aren't meant to show this to users. I think we want
| ) | ||
| return match | ||
| except TimeoutError: | ||
| log.warning( |
There was a problem hiding this comment.
We should probably make this an error, so we get it in Sentry if someone is messing with things.
| pattern, text, | ||
| ) | ||
| except Exception as e: | ||
| log.info('Error parsing regex: %s', e) |
There was a problem hiding this comment.
Similarly here -- seems bad to just log this exception. We probably want log.exception
Some times we may want to pass additional data around to the match and action functions. This is data that isn't available in the version object. Like the extra data needed to match external versions. We also need to return the response of the action, this is to allow the caller know what happened in the action. This was extracted from #7664
Some times we may want to pass additional data around to the match and action functions. This is data that isn't available in the version object. Like the extra data needed to match external versions. We also need to return the response of the action, this is to allow the caller know what happened in the action. This was extracted from #7664
|
I have split this into 2 PRs
There would be third if we agree with #7653 (comment) After those PRs are merged I'll update this one, the changes should be more small :) |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
Today I had a user request wanting to "exclude PR builds on matching branch names". As an example, they don't want to build PR when the branch starts with |
that should be supported with this PR, yeah. |
Allow to build an external version based on its source and base branch.
this was done, so we can know if the build was triggered or not for external versions.
but if the user has at least one, we use the rules to trigger the build.
Maybe wait for the new templates? Or maybe not as this is modifying just the js code.
Ref #7653