-
Notifications
You must be signed in to change notification settings - Fork 6
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
chore(ci-issues): add junit upload CLI #550
base: main
Are you sure you want to change the base?
Conversation
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🔴 🔎 ReviewsThis rule is failing.
🟢 Changelog requirementsWonderful, this rule succeeded.
🟢 Enforce conventional commitWonderful, this rule succeeded.Make sure that we follow https://www.conventionalcommits.org/en/v1.0.0/
|
85cfbf3
to
bb8d169
Compare
Pull request has been modified.
bb8d169
to
8744c22
Compare
2f24b3d
to
003113a
Compare
Adds the cli `mergify ci-issues junit-upload` to upload JUnit xml reports to CI Issues. Fixes MRGFY-4339
003113a
to
e9af926
Compare
"-s", | ||
help="Head SHA of the triggered job", | ||
required=True, | ||
envvar="HEAD_SHA", |
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 go further and load default depending on where the client is run (Github Action or Circle). Otherwise, it means if we add a new CLI option or we want to update an existing one, we have to update the GitHub actions and the Circle CI orb.
help="URL of the Mergify API", | ||
required=True, | ||
envvar="MERGIFY_API_SERVER", | ||
default="https://api.mergify.com/v1", |
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.
default="https://api.mergify.com/v1", | |
default="https://api.mergify.com/", |
I feel that should be the root server here, cc @sileht
"-t", | ||
help="CI Issues Application Key", | ||
required=True, | ||
envvar="MERGIFY_CI_ISSUES_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.
I think the token is global right now, so should we use something like MERGIFY_TOKEN
? cc @sileht
"-p", | ||
help="CI provider", | ||
default=None, | ||
envvar="CI_PROVIDER", |
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 does not seem exported by any CI, what's the point?
The default should be smart and autodetect the CI based on env vars.
"-r", | ||
help="Repository full name (owner/repo)", | ||
required=True, | ||
envvar="REPOSITORY", |
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 mean GITHUB_REPOSITORY
more likely?
@@ -0,0 +1,82 @@ | |||
import click |
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.
Rename everything to "CI" not "ci issues"
data=form_data, | ||
files=files_to_upload, | ||
) | ||
|
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.
handle status code != 200?
) | ||
|
||
|
||
def get_files_to_upload( |
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 only used once in upload so put it there?
especially since this just uploads XML file blindly so it really is not "utils" at all.
# TODO(leo): stream the files instead of loading them | ||
with file_path.open("rb") as f: | ||
files_to_upload.append( | ||
("files", (file_path.name, f.read(), "application/xml")), |
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.
Correct me if I'm wrong but it does not seem like you do not have to read the file content explicitly but you can just pass a file object to httpx.
That should prevent loading a lot of things in memory for nothing.
event_hooks["request"].insert(0, log_httpx_request) | ||
event_hooks["response"].insert(0, log_httpx_response) |
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 we share this debug thing too?
Adds the cli
mergify ci-issues junit-upload
to upload JUnit xml reports to CI Issues.Fixes MRGFY-4339