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.

Sign up for GitHub

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

Jump to bottom

etcd: Add golvuncheck for 3.4, 3.5 #32801

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

etcd: Add golvuncheck for 3.4, 3.5 #32801

wants to merge 1 commit into from

Conversation

ArkaSaha30
Copy link
Member

@ArkaSaha30 ArkaSaha30 commented Jun 20, 2024 •
edited
Loading

This commit will run pull-etcd-govulncheck for release-3.4, release-3.5 branch in addition to main.
Issue link: #32754

cc @ivanvc

@ArkaSaha30
Add golvuncheck for 3.4, 3.5
045214c 
This commit will run pull-etcd-govulncheck
for release-3.4, release-3.5 branch in addition to main.

Signed-off-by: ArkaSaha30 <arkasaha30@gmail.com>
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 20, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ArkaSaha30
Once this PR has been reviewed and has the lgtm label, please assign jmhbnz for approval. For more information see the Kubernetes Code Review Process.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jun 20, 2024
@k8s-ci-robot k8s-ci-robot added area/config Issues or PRs related to code in /config area/jobs sig/etcd Categorizes an issue or PR as relevant to SIG Etcd. sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Jun 20, 2024
ahrtr
ahrtr reviewed Jun 20, 2024
View reviewed changes
config/jobs/etcd/etcd-presubmits.yaml
Comment on lines +126 to +127
- release-3.5
- release-3.4
Copy link
Member

Choose a reason for hiding this comment

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

It seems that 3.5 and 3.4 do not have run-govulncheck in Makefile?

Copy link
Member

Choose a reason for hiding this comment

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

cc @ivanvc

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems that 3.5 and 3.4 do not have run-govulncheck in Makefile?

Yes, it needs to be backported

Copy link
Member Author

Choose a reason for hiding this comment

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

Since there is no test.sh we can add the govuln_pass() function in test_lib.sh for 3.4, 3.5 branches.

Copy link
Member Author

Choose a reason for hiding this comment

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

OR
we can fix the presubmit to not depend on Makefile and pass the direct commands to run

Copy link
Member

Choose a reason for hiding this comment

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

Since there is no test.sh we can add the govuln_pass() function in test_lib.sh for 3.4, 3.5 branches.

For release-3.5 test.sh is at the root project directory not in scripts/. Imo we should be backporting the Makefile recipe along with changes to test.sh so tests are run in a uniform way where possible. Otherwise we create cognitive burden when switching between working on different branches.

@ivanvc
Copy link
Member

ivanvc commented Jun 21, 2024

@jmhbnz, do we want to migrate jobs from branches other than main to prow? I haven't opened pull requests that run for 3.4/3.5 mainly because the Makefiles as stated above, are not always 100% compatible, and also because I haven't seen an effort to migrate these two branches.

@jmhbnz
Copy link
Member

jmhbnz commented Jun 21, 2024

@jmhbnz, do we want to migrate jobs from branches other than main to prow? I haven't opened pull requests that run for 3.4/3.5 mainly because the Makefiles as stated above, are not always 100% compatible, and also because I haven't seen an effort to migrate these two branches.

Good question - We could discuss at community meeting potentially or cc @ahrtr and @serathius for async decision. Personally I would be in favor.

@ahrtr
Copy link
Member

ahrtr commented Jun 21, 2024

do we want to migrate jobs from branches other than main to prow?

I do not see a reason not to do it. So yes to me.

@ivanvc
Copy link
Member

ivanvc commented Jun 21, 2024

Link to etcd-io/etcd#18173.

Regardless of the previous comment and outcome. I think we're ready to drop the optional: true and tune the requested CPU/memory. I was going to do it in a separate pull request, but I think it's fine if you update the values. The CPU should be at 4, and we can lower the memory to 1Gi, based on one week's usage (Grafana).

@BenTheElder BenTheElder changed the title Add golvuncheck for 3.4, 3.5 etcd: Add golvuncheck for 3.4, 3.5 Jun 21, 2024
@ivanvc ivanvc mentioned this pull request Jun 28, 2024
Merged
@ivanvc
Copy link
Member

ivanvc commented Aug 7, 2024 •
edited
Loading

We discussed this issue at the August 1st's etcd triage meeting. I'm just bumping this with a comment so we don't forget.

@ArkaSaha30, do you have the capacity to do the backports?

@ArkaSaha30
Copy link
Member Author

We discussed this issue at the August 1st's etcd triage meeting. I'm just bumping this with a comment so we don't forget.

@ArkaSaha30, do you have the capacity to do the backports?

Yess @ivanvc, I plan to take it up today or tomorrow. Slightly tied up with downstream work.

@ivanvc
Copy link
Member

ivanvc commented Aug 8, 2024

No worries, @ArkaSaha30. No rush at all. I was making sure that we didn't forget about this open issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahrtr ahrtr ahrtr left review comments

@jmhbnz jmhbnz jmhbnz left review comments

@serathius serathius Awaiting requested review from serathius

Assignees
No one assigned
Labels
area/config Issues or PRs related to code in /config area/jobs cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. sig/etcd Categorizes an issue or PR as relevant to SIG Etcd. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ArkaSaha30 @k8s-ci-robot @ivanvc @jmhbnz @ahrtr