-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Changing API flow description #17960
Conversation
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, soltysh The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
/retest |
1 similar comment
/retest |
|
||
2. The next step includes updating the [openshit/client-go](https://github.com/openshift/client-go/) | ||
with the changes from step 1, since it vendors it. To do so run `make update-deps` to pick up | ||
the changes from step 1 and then run `make generate` to update the client code with necessary |
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 whether the changes in step 1 need to be merged already.
[@openshift/sig-master](https://github.com/orgs/openshift/teams/sig-master) for a review. | ||
|
||
3. The final step happens in [openshift/origin](https://github.com/openshift/origin/) repository. | ||
As previously, run `make update-deps` to pick up the changes from previous two steps. Afterwards |
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.
Same comment as above.
Automatic merge from submit-queue. |
|
||
NOTE: It may happen that during `make update-deps` step you will pick up the changes introduced | ||
by someone else in his PR. In that case sync with the other PR's author and include his changes | ||
in your PR noting the fact to your reviewer. |
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 must find a way to address/avoid this, it is untenable if the changes are big.
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 must find a way to address/avoid this, it is untenable if the changes are big.
Simple roundtripping isn't bad to add in general and there really aren't that many changes. While not ideal, I don't think it's an area that is untenable at the moment.
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 (the master team) had a lengthy discussion about it and we've decided that with the amount of changes it's not worth the effort to avoid it. Besides there are very easy ways to bypass them 😉
@mfojtik you asked for it
@deads2k since you're the author