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
Moving pkg/kubectl/util/storage to staging #79204
Conversation
Hi @pswica. Thanks for your PR. I'm waiting for a kubernetes 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/test-infra repository. |
/ok-to-test |
staging/publishing/rules.yaml
Outdated
dependencies: | ||
- repository: apimachinery | ||
branch: master | ||
- repository: client-go |
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 looks like client-go is not a dependency yet, can we remove it?
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 will fix this too (I was just copying rules from another PR--there are lots for this big move). Thanks again for your review
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 re-added this. I think with all of our code moves and re-merging this is now necessary, otherwise tests start failing.
staging/publishing/rules.yaml
Outdated
- source: | ||
branch: release-1.15 | ||
dir: staging/src/k8s.io/kubectl | ||
name: release-1.15 | ||
go: 1.12.5 | ||
|
||
dependencies: | ||
- repository: apimachinery |
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.
nit: to be alphabetically consistent, would be better to put api
over apimachinery
😛
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.
Great catch! Thanks for reviewing, I will change this.
Please also add kubectl to https://github.com/kubernetes/kubernetes/blob/master/staging/publishing/import-restrictions.yaml |
@nikhita can you help me understand this portion? I'm still new to this codebase and I don't want to update this .yaml in the wrong places.
|
Sure! We want to make sure that certain (staging) packages don't end up importing other staging packges. For example, apimachinery should never depend on another staging repo. These import restrictions are enforced by that yaml file. For instance, in this: kubernetes/staging/publishing/import-restrictions.yaml Lines 33 to 37 in 4e0b764
It shows that apimachinery should only depend on kube-openapi and klog (they have the For kubectl, we'll need to add something like https://github.com/kubernetes/kubernetes/pull/79207/files#diff-2c52da2e345c051a2fed16d27e7702a7 to make sure we don't accidentally add any unwanted imports. :) |
@nikhita thanks so so much for your thoughtful reviews, I tried to make the suggested changes. You are the best. |
No, no, thank you for making these changes! ❤️ |
/hold cancel |
/priority important-soon |
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.
Please remove "staging/publishing/rules.yaml" from this PR. We shouldn't have to change this file anymore. Your changes to this file are only indentation changes, making the indentation inconsistent.
/uncc @dchen1107 |
Okay I changed that file. I think that was a remnant of me editing that file before all the merging. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pswica, soltysh 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 |
/retest |
/lgtm |
/retest |
@pswica: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. |
/retest Review the full test history for this PR. Silence the bot with an |
Moving pkg/kubectl/util/storage to staging Kubernetes-commit: b84d5b8
/kind cleanup
/sig cli
/area kubectl