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

Bump the version of go-restful to 2.16.0 #8092

Merged
merged 1 commit into from
Jul 16, 2022
Merged

Bump the version of go-restful to 2.16.0 #8092

merged 1 commit into from
Jul 16, 2022

Conversation

Barakmor1
Copy link
Member

@Barakmor1 Barakmor1 commented Jul 13, 2022 •
edited

Signed-off-by: bmordeha bmodeha@redhat.com

What this PR does / why we need it:
Bump the version of emicklei/go-restful from 2.15.0 to 2.16.0 to fix emicklei/go-restful#493
See emicklei/go-restful#489 (comment).
(emicklei/go-restful#503)

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:
Modified files ( not auto generated ) :
go.mod
staging/src/kubevirt.io/client-go/go.mod

Release note:

Bump the version of emicklei/go-restful from 2.15.0 to 2.16.0

@kubevirt-bot kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/L labels Jul 13, 2022
@kubevirt-bot kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jul 13, 2022
@iholder101
Copy link
Contributor

/cc

xpivarc
xpivarc reviewed Jul 13, 2022
View reviewed changes
staging/src/kubevirt.io/client-go/go.mod Outdated
@@ -71,6 +71,7 @@ require (
)

replace (
github.com/emicklei/go-restful => github.com/emicklei/go-restful v2.16.0+incompatible
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this?

Copy link
Member Author

@Barakmor1 Barakmor1 Jul 13, 2022 •
edited

Choose a reason for hiding this comment

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

This cause that dependencies that use go-restful to use the library locally ( locally we use v2.16.0 )
without it go-restful-openapi and other libraries use old versions of go-restful

found the solutions in here:
https://stackoverflow.com/questions/69825533/why-does-go-sum-include-so-many-older-packages

and here some information about replace:
https://thewebivore.com/using-replace-in-go-mod-to-point-to-your-local-module/

Copy link
Member

Choose a reason for hiding this comment

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

I went through your references but I am not sure what I should focus on. Can you be more specific? I think what you are looking for and what will contradict is how Go selects the library version it should use (ref )

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks!
Will update soon

@Barakmor1
Copy link
Member Author

/retest

1 similar comment
@Barakmor1
Copy link
Member Author

/retest

xpivarc
xpivarc reviewed Jul 14, 2022
View reviewed changes
Copy link
Member

@xpivarc xpivarc left a comment

Choose a reason for hiding this comment

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

/approve

@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: xpivarc

The full list of commands accepted by this bot can be found here.

The pull request process is described 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

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 14, 2022
@iholder101
Copy link
Contributor

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 14, 2022
@Barakmor1
Copy link
Member Author

/retest

1 similar comment
@Barakmor1
Copy link
Member Author

/retest

EdDev
EdDev reviewed Jul 14, 2022
View reviewed changes
Copy link
Member

@EdDev EdDev left a comment

Choose a reason for hiding this comment

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

I would prefer if the git log history is populated with meaningful commit messages.
Please consider adding the details of why this change is done to the commit message as well.

@Barakmor1
Copy link
Member Author

/retest

@iholder101
Copy link
Contributor

iholder101 commented Jul 14, 2022 •
edited

I would prefer if the git log history is populated with meaningful commit messages. Please consider adding the details of why this change is done to the commit message as well.

It's a bit off-topic for this PR :) but I'm having hard time to understand why this matters.

PR description is way more readable, can update after the PR merges, can contain images, proper links and so on. How would we benefit from duplicating this description into the commit messages? Especially when the PR description updates and the commit message doesn't (this can also happen before the PR merges - it's easy to remember to update the PR and forget updating the commit messages). Also, when you git log, you would easily find the PR number and can search for it and find the description.

I know it's an unpopular opinion, but I honestly don't think that saving a few seconds of searching the PR number in github is worth duplicating description that later can contradict or at least be different then what's explained at the PR.

@iholder101
Copy link
Contributor

@EdDev
By the way, today we have these commit messages for merged PRs:

    Merge pull request #8065 from lyarwood/virt-operator-remove-subresource-perms
    
    virt-operator: Remove explicit subresources permissions

If we would just add the PR description, then we would have a way of having these descriptions inside git log.
But, as said before, I'm not sure if it's a good practice at all. But definitely better than documenting twice imho

@EdDev
Copy link
Member

EdDev commented Jul 14, 2022

I know it's an unpopular opinion, but I honestly don't think that saving a few seconds of searching the PR number in github is worth duplicating description that later can contradict or at least be different then what's explained at the PR.

There are the regular "do not depend on the provider, it will lock you in even to learn about the history".
I'm writing my code locally, and I am recording what I am doing (in code) and why (in a commit).
This helps me, but also helps others, to understand why something was changed. I use git blame frequently to learn why
and in what context a change was introduced.

Requiring me to have internet access, open a browser, copy paste the commit sha, search for it on GitHub and then reach the relevant PR is work I would prefer to avoid. (I am sure there are other ways to do this)

I think it is more accurate to place the knowledge in the VCS and not in the provider.
This should save us from cases where the provider is down, I am in a flight to my favorite vacation resort and still want to work, or when my home/office internet is down.

Additionally, when there are several commits in a PR, this also assists in the review process, to understand what the commit intention, so that it can be compared with what was actually done.

I think the effort is not that high, I just copy the commit message content to the PR.

If we would just add the PR description, then we would have a way of having these descriptions inside git log.
But, as said before, I'm not sure if it's a good practice at all. But definitely better than documenting twice imho

Merge commits are a different beast, I personally do not like this merge strategy which messes the history (IMO).
I have no opinions about the PR text being moved into the merge commit message. It does not help me nor hurt me in my workflow.

@kubevirt-bot kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 14, 2022
Bump the version of go-restful to 2.16.0
2cbc394 
Because of a security issue in go-restful v2.15.0
emicklei/go-restful#503
Signed-off-by: bmordeha <bmodeha@redhat.com>
@Barakmor1
Copy link
Member Author

@EdDev Done.

@iholder101
Copy link
Contributor

I know it's an unpopular opinion, but I honestly don't think that saving a few seconds of searching the PR number in github is worth duplicating description that later can contradict or at least be different then what's explained at the PR.

There are the regular "do not depend on the provider, it will lock you in even to learn about the history". I'm writing my code locally, and I am recording what I am doing (in code) and why (in a commit). This helps me, but also helps others, to understand why something was changed. I use git blame frequently to learn why and in what context a change was introduced.

Requiring me to have internet access, open a browser, copy paste the commit sha, search for it on GitHub and then reach the relevant PR is work I would prefer to avoid. (I am sure there are other ways to do this)

I think it is more accurate to place the knowledge in the VCS and not in the provider. This should save us from cases where the provider is down, I am in a flight to my favorite vacation resort and still want to work, or when my home/office internet is down.

Additionally, when there are several commits in a PR, this also assists in the review process, to understand what the commit intention, so that it can be compared with what was actually done.

I think the effort is not that high, I just copy the commit message content to the PR.

If we would just add the PR description, then we would have a way of having these descriptions inside git log.
But, as said before, I'm not sure if it's a good practice at all. But definitely better than documenting twice imho

Merge commits are a different beast, I personally do not like this merge strategy which messes the history (IMO). I have no opinions about the PR text being moved into the merge commit message. It does not help me nor hurt me in my workflow.

I understand your point, and I think your opinion is way more popular, but I still don't agree with it :)

If we copy the PR description to our merge commits you're not dependent on internet connection etc. And to be honest, the case of working without an opened browser and an internet connection is very very very rare (if ever happened at all), at least for me.

By the way, I also use git blame a lot. I usually go straight to the PR description, then starting to look at the relevant commits I need. But if the PR description is good enough, I usually don't need anything except for the commit title. There are cases where implementation details and such are important at the commit itself, but it's pretty rare in my opinion. In any case, these details should usually reside only at the commit level and not the PR description.

I think the effort is not that high, I just copy the commit message content to the PR.

It happens quite a lot that the implementation changes after some review comments. In these cases I often squash commits, reverse their order, and this would cause me to change the description as well. Instead, I find it a lot more comfortable to have the description of the whole PR (which is a cohesive unit) at the PR level.

But we can agree to disagree on that :)

EdDev
EdDev approved these changes Jul 14, 2022
View reviewed changes
Copy link
Member

@EdDev EdDev left a comment

Choose a reason for hiding this comment

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

Thanks!

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 14, 2022
@Barakmor1
Copy link
Member Author

/retest

1 similar comment
@Barakmor1
Copy link
Member Author

/retest

@kubevirt-commenter-bot
Copy link

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@dhiller
Copy link
Contributor

dhiller commented Jul 15, 2022

/hold for getting the clone fix PR in

FYI @brianmcarey

@kubevirt-bot kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 15, 2022
@dhiller
Copy link
Contributor

dhiller commented Jul 15, 2022

/unhold

@kubevirt-bot kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 15, 2022
@dhiller
Copy link
Contributor

dhiller commented Jul 15, 2022

/unhold

@kubevirt-commenter-bot
Copy link

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

1 similar comment
@kubevirt-commenter-bot
Copy link

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@Barakmor1
Copy link
Member Author

/retest

@kubevirt-bot
Copy link
Contributor

kubevirt-bot commented Jul 16, 2022 •
edited

@Barakmor1: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-kubevirt-fossa 2cbc394 link false /test pull-kubevirt-fossa

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/test-infra repository. I understand the commands that are listed here.

@kubevirt-commenter-bot
Copy link

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@kubevirt-bot kubevirt-bot merged commit c4e5338 into kubevirt:main Jul 16, 2022
@Barakmor1
Copy link
Member Author

/cherrypick release-0.49

@kubevirt-bot
Copy link
Contributor

@Barakmor1: #8092 failed to apply on top of branch "release-0.49":

Applying: Bump the version of go-restful to 2.16.0 Because of a security issue in go-restful v2.15.0 https://github.com/emicklei/go-restful/pull/503 Signed-off-by: bmordeha <bmodeha@redhat.com>
Using index info to reconstruct a base tree...
M	go.mod
M	go.sum
M	staging/src/kubevirt.io/client-go/go.mod
M	staging/src/kubevirt.io/client-go/go.sum
M	vendor/github.com/emicklei/go-restful/CHANGES.md
M	vendor/github.com/emicklei/go-restful/response.go
M	vendor/github.com/emicklei/go-restful/route.go
M	vendor/github.com/emicklei/go-restful/web_service.go
M	vendor/modules.txt
Falling back to patching base and 3-way merge...
Auto-merging vendor/modules.txt
CONFLICT (content): Merge conflict in vendor/modules.txt
Auto-merging vendor/github.com/emicklei/go-restful/web_service.go
Auto-merging vendor/github.com/emicklei/go-restful/route.go
Auto-merging vendor/github.com/emicklei/go-restful/response.go
CONFLICT (content): Merge conflict in vendor/github.com/emicklei/go-restful/response.go
Auto-merging vendor/github.com/emicklei/go-restful/CHANGES.md
CONFLICT (content): Merge conflict in vendor/github.com/emicklei/go-restful/CHANGES.md
Auto-merging staging/src/kubevirt.io/client-go/go.sum
CONFLICT (content): Merge conflict in staging/src/kubevirt.io/client-go/go.sum
Auto-merging staging/src/kubevirt.io/client-go/go.mod
CONFLICT (content): Merge conflict in staging/src/kubevirt.io/client-go/go.mod
Auto-merging go.sum
CONFLICT (content): Merge conflict in go.sum
Auto-merging go.mod
CONFLICT (content): Merge conflict in go.mod
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Bump the version of go-restful to 2.16.0 Because of a security issue in go-restful v2.15.0 https://github.com/emicklei/go-restful/pull/503 Signed-off-by: bmordeha <bmodeha@redhat.com>
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick release-0.49

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/test-infra repository.

@Barakmor1
Copy link
Member Author

/cherrypick release-0.53

@kubevirt-bot kubevirt-bot mentioned this pull request Jul 17, 2022
Closed
@kubevirt-bot
Copy link
Contributor

@Barakmor1: new pull request created: #8108

In response to this:

/cherrypick release-0.53

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/test-infra repository.

Barakmor1 pushed a commit to Barakmor1/kubevirt that referenced this pull request Jul 17, 2022
back port :Bump the version of go-restful to 2.16.0
e74ac10 
Original PR: kubevirt#8092
Because of a security issue in go-restful v2.15.0
Signed-off-by: bmordeha <bmodeha@redhat.com>
@Barakmor1 Barakmor1 mentioned this pull request Jul 17, 2022
Merged
Barakmor1 pushed a commit to Barakmor1/kubevirt that referenced this pull request Jul 17, 2022
@kubevirt-bot
Merge pull request kubevirt#8092 from Barakmor1/go_restful_update_new
d6b6cdb 
Bump the version of `go-restful` to 2.16.0
Because of a security issue in go-restful v2.15.0
Signed-off-by: bmordeha <bmodeha@redhat.com>
@Barakmor1 Barakmor1 mentioned this pull request Jul 17, 2022
Merged
jean-edouard pushed a commit to jean-edouard/kubevirt that referenced this pull request Oct 4, 2022
@jean-edouard
back port :Bump the version of go-restful to 2.16.0
9110d45 
Original PR: kubevirt#8092
Because of a security issue in go-restful v2.15.0
Signed-off-by: bmordeha <bmodeha@redhat.com>

(cherry picked from commit e74ac10)
Signed-off-by: Jed Lejosne <jed@redhat.com>
@jean-edouard jean-edouard mentioned this pull request Oct 4, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xpivarc xpivarc xpivarc left review comments

@EdDev EdDev EdDev approved these changes

@alicefr alicefr Awaiting requested review from alicefr

@iholder101 iholder101 Awaiting requested review from iholder101

Assignees

@EdDev EdDev

@iholder101 iholder101

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@Barakmor1 @iholder101 @kubevirt-bot @EdDev @kubevirt-commenter-bot @dhiller @xpivarc