Skip to content
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

test: add release info to xml file before uploading to obj storage #437

Merged
merged 2 commits into from Dec 6, 2023

Conversation

ykim-1
Copy link
Contributor

@ykim-1 ykim-1 commented Dec 5, 2023

📝 Description

Problem: For some context, the script that is used to upload test report from OBJ storage to TOD is currently calling GitHub API endpoints from ECP Test VM to get the release info for corresponding repository. However recently there has been a rate limiting issue that is causing an error calling endpoint from that particular VM. E.g.
Error: 403 Client Error: rate limit exceeded for url: https://api.github.com/repos/linode/terraform-provider-linode/releases/latest linode-terraform None

✔️ How to Test

Monitor TOD after merged to dev

📷 Preview

If applicable, include a screenshot or code snippet of this change. Otherwise, please remove this section.

@ykim-1 ykim-1 requested a review from a team as a code owner December 5, 2023 22:48
@ykim-1 ykim-1 requested review from lgarber-akamai, vshanthe and zliang-akamai and removed request for a team December 5, 2023 22:48
Copy link
Contributor

@lgarber-akamai lgarber-akamai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me other than one small typo 👍

scripts/add_to_xml_test_report.py Outdated Show resolved Hide resolved
@@ -1,11 +1,38 @@
import argparse
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side note: Given we re-use these scripts in a bunch of DX-maintained repos, I wonder if it would make sense to move them into their own repo and pull them into each implementation using submodules.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's a good idea if these scripts are identical.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that is a good idea, only thing different is the latest_release_url which can be a list or an array that can be selected based on the repo. I can create some tickets to address this.

Co-authored-by: Lena Garber <114949949+lgarber-akamai@users.noreply.github.com>
Copy link
Contributor

@zliang-akamai zliang-akamai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@ykim-1 ykim-1 merged commit 2fe6f94 into linode:main Dec 6, 2023
3 checks passed
ykim-1 added a commit that referenced this pull request Dec 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants