-
Notifications
You must be signed in to change notification settings - Fork 306
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
"This is now live" automatic notifications #715
base: main
Are you sure you want to change the base?
Conversation
|
||
|
||
def get_linked_pr(commit: str, token: str) -> dict: | ||
pr = get(f"repos/{OWNER_REPO}/commits/{commit}/pulls", token=token) |
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.
Technically this is should be a paginated get, but if we find a PR with 30 or more linked issues.. Wow
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.
Also, the GraphQL query only lists the first 10, so there won't be more than that in the response, which fit in a single page
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.
Can graphql query on label status? I think that finding results that haven't been tagged live
might be safer and easier on humans to find if it goes wrong, instead of having to look inside all the comments inside each PR
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 is now ready for review. Needs some aws backend work to get the ssm and such
|
||
|
||
def get_linked_pr(commit: str, token: str) -> dict: | ||
pr = get(f"repos/{OWNER_REPO}/commits/{commit}/pulls", token=token) |
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.
Also, the GraphQL query only lists the first 10, so there won't be more than that in the response, which fit in a single page
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.
Some comments here - looks amazing; happy to discuss further. Thanks for this! Relatively small amount of code too, which is amazing :)
|
||
|
||
def get_linked_pr(commit: str, token: str) -> dict: | ||
pr = get(f"repos/{OWNER_REPO}/commits/{commit}/pulls", token=token) |
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.
Can graphql query on label status? I think that finding results that haven't been tagged live
might be safer and easier on humans to find if it goes wrong, instead of having to look inside all the comments inside each PR
a133c51
to
64b131b
Compare
Addresses PR reviews
e49457d
to
182b7cd
Compare
The failing test comes from the branch being a bit outdated, nothing to worry about |
Just ran |
In case of error, the program will break either way, but we make sure we hide the token secret
This could be awesome. Any chance of resurrecting this @RubenRBS ? |
There is still some work to be done, but the gist of it is:
ce builds set_current ...
store the oldest non-notified commit, skipping if something is already storedce environment refresh
(and laterce instances restart
) get the currently deployed version, and:This would need a new ssm in aws, and I need to test it in case where no key exists (Which should be the case of env refreshes for the same version!)