-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Auto dependency bump #11557
base: master
Are you sure you want to change the base?
Auto dependency bump #11557
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kokyhm The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Hi @kokyhm. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
/ok-to-test |
First: This needs some comments to have a rough idea of what the code does without too much trouble. I have a couple of comments at first glance:
|
Thank you @VannTen for the comments. Hope my refactor address comments you have provided. Key changes:
Speed:
Thanks again and looking forward to your comments. |
Hum, 20 minutes is a lot. It's also not sure we'd run this on GitHub workers, given our CI is in gitlab and the larger kubernetes-sigs in prow, so I'd rather not take that for granted.
(The most gains I've seen previously was to download the hashes instead of the binaries, are you doing that ?)
I'll try to look at the code more in detail, but 500 lines of python is pretty big to review, so it could take a while until I have the time to look properly at this.
Also, I wanted to mention this the other time but forgot: I had some wip stuff on that subject started some time ago, feel free to look at it (if you want) https://github.com/VannTen/kubespray/tree/wip/download_graphql
|
Let's recap:
What have i found, do not hardcode kubespray versions... |
I do not get "free to look at it if you want". What does it mean? |
It means no more that than, I've done that some times ago, figured you might get some inspiration from it.
A usecase for what ? A fast script ?
The usecase is for the maintainers who are going to debug the script when it will have bugs.
Besides that, the script is big, so it's gonna take a while to review. That's always the case for big PRs, that's why an iterative approach usually is faster.
|
Thank you for the comments. Was rushing it, sorry about that. |
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.
First some preamble:
This should really be split in several parts (at least 2):
- script to download
- automation to run the script.
(I can't review everything in details, there is really too much, but I've left some general comments).
@@ -0,0 +1,221 @@ | |||
ARCHITECTURES = ['arm', 'arm64', 'amd64', 'ppc64le'] |
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'd rather source this from the data (aka take the existing keys in the existing checksums)
version = tag.get('name', '') | ||
if stable_version_pattern.match(version): | ||
patch_versions.append(version) | ||
patch_versions.sort(key=lambda v: list(map(int, re.findall(r'\d+', v)))) # sort for checksum update |
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's not clear what you mean by sort for checkum update
.
}} | ||
}} | ||
""") | ||
|
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.
On the query.
First, you should really put it in a separate file, so it can be edited and run separately.
Secondly, you should use graphql variables instead of templating the query and templating and joining.
For instance something like this :
query($repoWithReleases: [ID!]!, $repoWithTags: [ID!]!) {
with_releases: nodes(ids: $repoWithReleases) {
... on Repository {
nameWithOwner
releases(first: 100) {
nodes {
tagName
isPrerelease
releaseAssets {
totalCount
}
}
}
}
}
with_tags: nodes(ids: $repoWithTags) {
... on Repository {
nameWithOwner
refs(refPrefix: "refs/tags/", last: 100) {
nodes {
name
}
}
}
}
}
cache_file = f'{component}-{arch}-{version}' | ||
if os.path.exists(f'cache/{cache_file}'): | ||
logging.info(f'Using cached file for {url_download}') | ||
return calculate_checksum(cache_file, sha_regex) | ||
try: | ||
response = session.get(url_download, timeout=10) | ||
response.raise_for_status() | ||
with open(f'cache/{cache_file}', 'wb') as f: | ||
f.write(response.content) | ||
logging.info(f'Downloaded and cached file for {url_download}') |
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 don't think we want to do caching (at least at start) It adds some footguns + I'd rather have a more predictable output from multiples "runners" (CI, locally, etc).
def update_readme(component, version): | ||
for i, line in enumerate(readme_data): | ||
if component in line and re.search(r'v\d+\.\d+\.\d+', line): | ||
readme_data[i] = re.sub(r'v\d+\.\d+\.\d+', version, line) | ||
logging.info(f'Updated {component} to {version} in README') | ||
break | ||
return readme_data |
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 first focus on the checksums, the README is another story.
@VannTen Thank you very much for your time and comments. (Un)fortunatelly, I was very busy last few months, that's why I did not participate in other tasks. Just checking POC from time to time to see how it works(found some minor bugs). I will address your comments at the start of the next year. Wish you all the best. |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Auto dependency bump
Kubespray uses newer software and the maintainers can save time.
Which issue(s) this PR fixes:
Fixes #10681
Special notes for your reviewer:
POC:
Actions
PRs
Known issues(YAML):
Does this PR introduce a user-facing change?: