-
Notifications
You must be signed in to change notification settings - Fork 462
OCPBUGS-5696: remove goutils from dependency tree #3480
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
OCPBUGS-5696: remove goutils from dependency tree #3480
Conversation
|
@cheesesashimi: This pull request references Jira Issue OCPBUGS-5696, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
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. |
31a7b87 to
a450b74
Compare
|
@cheesesashimi: This pull request references Jira Issue OCPBUGS-5696, which is invalid:
Comment DetailsIn response to this:
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. |
a450b74 to
d214971
Compare
d214971 to
31cbc9e
Compare
|
Did a first pass and everything makes sense, thanks for the quick fix! As a side note, this did annoyingly create 5 bugs: in 4.12->4.8 order, but no 4.13, so it seems that e.g. an option would be:
or
up to you which works better |
|
@cheesesashimi: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
Lets try the cherrypick magic as a first-pass. If we end up having a lot of difficulty, I'll do them manually. Also, isn't 4.13 the most recent (i.e., |
|
/cherrypick release-4.11 EDIT: One must specify each cherrypick individually. |
|
@cheesesashimi: once the present PR merges, I will cherry-pick it on top of release-4.11 in a new PR and assign it to you. DetailsIn response to this:
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. |
|
/cherrypick release-4.10 |
|
@cheesesashimi: once the present PR merges, I will cherry-pick it on top of release-4.10 in a new PR and assign it to you. DetailsIn response to this:
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. |
|
/cherrypick release-4.9 |
|
@cheesesashimi: once the present PR merges, I will cherry-pick it on top of release-4.9 in a new PR and assign it to you. DetailsIn response to this:
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. |
|
/cherrypick release-4.8 |
|
@cheesesashimi: once the present PR merges, I will cherry-pick it on top of release-4.8 in a new PR and assign it to you. DetailsIn response to this:
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. |
|
/retest required I wonder if this works |
|
@yuqi-zhang: The
The following commands are available to trigger optional jobs:
Use
DetailsIn response to this:
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. |
|
oh whoops that should have been 1 command but I do see the new required jobs, so I think we're good. I think this shouldn't require any pre-merge testing either right? Just removing some deps so CI coverage should be enough |
|
Yeah, I think this is all we need. |
yuqi-zhang
left a comment
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.
/lgtm
I don't think this needs pre-merge testing, since it doesn't change any behaviour really, just removes some dependencies and adds testing. Will let the regular bug verification process do the trick instead.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cheesesashimi, yuqi-zhang The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/jira refresh |
|
@cheesesashimi: This pull request references Jira Issue OCPBUGS-5696, which is invalid:
Comment DetailsIn response to this:
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. |
|
/jira refresh |
|
@cheesesashimi: This pull request references Jira Issue OCPBUGS-5696, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn response to this:
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. |
|
@cheesesashimi: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-5696 has been moved to the MODIFIED state. DetailsIn response to this:
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. |
|
@cheesesashimi: #3480 failed to apply on top of branch "release-4.9": DetailsIn response to this:
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. |
|
@cheesesashimi: #3480 failed to apply on top of branch "release-4.8": DetailsIn response to this:
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. |
|
@cheesesashimi: #3480 failed to apply on top of branch "release-4.11": DetailsIn response to this:
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. |
|
@cheesesashimi: #3480 failed to apply on top of branch "release-4.10": DetailsIn response to this:
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. |
|
/cherrypick release-4.12 |
|
@yuqi-zhang: #3480 failed to apply on top of branch "release-4.12": DetailsIn response to this:
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. |
- What I did
Analyzed the MCO to understand where goutils was being introduced as a dependency. I discovered that it was introduced via the sprig template helper library. We only make use of two functions within that library (which do not depend on goutils). This enabled me to do the following:
Note to reviewer: I put the dependency removal in a commit by itself for easier review.
- How to verify it
Run the unit test suite
- Description for the changelog
Remove goutils from our dependency tree