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

Use dedicated UnixUserID and UnixGroupID types #44714

Merged
merged 1 commit into from
May 5, 2017
Merged

Use dedicated UnixUserID and UnixGroupID types #44714

merged 1 commit into from
May 5, 2017

Conversation

jamiehannaford
Copy link
Contributor

@jamiehannaford jamiehannaford commented Apr 20, 2017 •
edited by caesarxuchao
Loading

What this PR does / why we need it:

DRYs up type definitions by using the dedicated types in apimachinery

Which issue this PR fixes

#38120

Release note:

UIDs and GIDs now use apimachinery types

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 20, 2017
@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Apr 20, 2017
@k8s-ci-robot
Copy link
Contributor

Hi @jamiehannaford. 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 @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

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.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 20, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@smarterclayton
Copy link
Contributor

@k8s-bot ok to test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 20, 2017
@smarterclayton
Copy link
Contributor

API change in general approved as per linked thread.

@jamiehannaford jamiehannaford mentioned this pull request Apr 26, 2017
Closed
@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 26, 2017
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 26, 2017
@jamiehannaford jamiehannaford force-pushed the unix_user_type branch 6 times, most recently from 949da60 to 3e5f194 Compare April 27, 2017 11:14
@jamiehannaford
Copy link
Contributor Author

@smarterclayton @yifan-gu @pmorie This is RFR. I know it's a ginormous changeset, but these API types touched lots of the codebase. If there's anything I can do to make reviewing easier (like breaking stuff up) please let me now!

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 29, 2017
@jamiehannaford
Copy link
Contributor Author

jamiehannaford commented May 2, 2017 •
edited
Loading

@smarterclayton @yifan-gu Is there any way to make this easier to review? Otherwise I might just close this PR, since it's unlikely folks with have time to review +100 files.

Once we've figured out a strategy, I can rebase and fix conflicts/tests.

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 2, 2017
@jamiehannaford
Copy link
Contributor Author

Hm, strange. The bazel build is failing, but when you click on the Details link it says Unable to load build details. When I run hack/verify-bazel.sh locally, no errors are reported, so everything seems up to date.

@thockin Any ideas what may be causing this? I've tried re-pushing and the same thing occurs.

@thockin
Copy link
Member

thockin commented May 5, 2017

@k8s-bot bazel test this

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented May 5, 2017 •
edited
Loading

@jamiehannaford: The following test(s) failed:

Test name Commit Details Rerun command
Jenkins GCE e2e e5f1c87aad1de9fbeb92bd6e963cd84e4c6bcf13 link @k8s-bot cvm gce e2e test this
Jenkins GCI GCE e2e e5f1c87aad1de9fbeb92bd6e963cd84e4c6bcf13 link @k8s-bot gci gce e2e test this
Jenkins non-CRI GCE e2e e5f1c87aad1de9fbeb92bd6e963cd84e4c6bcf13 link @k8s-bot non-cri e2e test this
Jenkins non-CRI GCE Node e2e e5f1c87aad1de9fbeb92bd6e963cd84e4c6bcf13 link @k8s-bot non-cri node e2e test this

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.

@thockin
Copy link
Member

thockin commented May 5, 2017

/lgtm

Comments from Reviewable

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 5, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jamiehannaford, thockin

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@thockin
Copy link
Member

thockin commented May 5, 2017

@ixdy - bazel build fails immediately, no data.

@ixdy
Copy link
Member

ixdy commented May 5, 2017

issue with prow.
@k8s-bot bazel test this

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 44590, 44969, 45325, 45208, 44714)

@k8s-github-robot k8s-github-robot merged commit 5b3d0bb into kubernetes:master May 5, 2017
@ericchiang ericchiang mentioned this pull request Jun 14, 2017
Closed
@mengqiy mengqiy mentioned this pull request Jun 16, 2017
Closed
@lavalamp
Copy link
Member

@smarterclayton, @thockin: This PR included non-api types in the api type tree--that would have been good to call out in the review.

@smarterclayton
Copy link
Contributor

Hrm, we already have types.UID

@lavalamp
Copy link
Member

I'm adding openapi definitions because that seems easier than unbreaking this.

@lavalamp
Copy link
Member

Workaround: #47753

@mbohlool
Copy link
Contributor

mbohlool commented Jun 20, 2017 •
edited
Loading

Look like the OpenAPI changes is not the source of compatibility issue. It should be an old swagger 1.2 change. I will look into a solution.

On things to call out in the review, can we make it a requirement to have separate commits for generated code. It is hard to find manual code inside this PR (I assume all types.go files, but I cannot be sure).

@mbohlool mbohlool mentioned this pull request Jun 20, 2017
Closed
mbohlool added a commit to mbohlool/kubernetes that referenced this pull request Jun 20, 2017
@mbohlool
Revert kubernetes#44714 (only types.go files)
26611fe
@mbohlool mbohlool mentioned this pull request Jun 21, 2017
Merged
@caesarxuchao
Copy link
Member

I removed the release note because the PR is reverted

@caesarxuchao
Copy link
Member

/release-note-none

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jun 21, 2017
@dchen1107 dchen1107 mentioned this pull request Jun 21, 2017
Closed
k8s-github-robot pushed a commit that referenced this pull request Jun 21, 2017
Merge pull request #47824 from mbohlool/revert2
03014f4 
Automatic merge from submit-queue (batch tested with PRs 47851, 47824, 47858, 46099)

Revert 44714 manually

#44714 broke backward compatibility for old swagger spec that kubectl still uses. The decision on #47448 was to revert this change but the change was not automatically revertible. Here I semi-manually remove all references to UnixUserID and UnixGroupID and updated generated files accordingly.

Please wait for tests to pass then review that as there may still be tests that are failing.

Fixes #47448

Adding release note just because the original PR has a release note. If possible, we should remove both release notes as they cancel each other.

**Release note**: (removed by caesarxuchao)

UnixUserID and UnixGroupID is reverted back as int64 to keep backward compatibility.
dims pushed a commit to dims/kubernetes that referenced this pull request Feb 8, 2018
Merge pull request kubernetes#44714 from jamiehannaford/unix_user_type
8273042 
Automatic merge from submit-queue (batch tested with PRs 44590, 44969, 45325, 45208, 44714)

Use dedicated UnixUserID and UnixGroupID types

**What this PR does / why we need it**:

DRYs up type definitions by using the dedicated types in apimachinery 

**Which issue this PR fixes**

kubernetes#38120

**Release note**:
```release-note
UIDs and GIDs now use apimachinery types
```
dims pushed a commit to dims/kubernetes that referenced this pull request Feb 8, 2018
Merge pull request kubernetes#47824 from mbohlool/revert2
8335505 
Automatic merge from submit-queue (batch tested with PRs 47851, 47824, 47858, 46099)

Revert 44714 manually

kubernetes#44714 broke backward compatibility for old swagger spec that kubectl still uses. The decision on kubernetes#47448 was to revert this change but the change was not automatically revertible. Here I semi-manually remove all references to UnixUserID and UnixGroupID and updated generated files accordingly.

Please wait for tests to pass then review that as there may still be tests that are failing.

Fixes kubernetes#47448

Adding release note just because the original PR has a release note. If possible, we should remove both release notes as they cancel each other.

**Release note**: (removed by caesarxuchao)

UnixUserID and UnixGroupID is reverted back as int64 to keep backward compatibility.
@php-coder php-coder mentioned this pull request May 4, 2018
Merged
k8s-github-robot pushed a commit that referenced this pull request May 4, 2018
Merge pull request #63438 from php-coder/psp_unix_user_and_group_id
d4b4f5a 
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Use single struct in internal type to reduce difference with external type

**What this PR does / why we need it**:
This PR reduces the differences between internal and external types by removing leftovers after revert of one of the changes.

**Special notes for your reviewer**:
We had `IDRange` in both types prior 9440a68 commit (#44714) that splitted it into `UserIDRange`/`GroupIDRange`. Later, in c91a12d commit (#47824) we had to revert these changes because they broke backward compatibility but `UserIDRange`/`GroupIDRange` structs were left in the internal type.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thockin thockin thockin left review comments

Assignees

@smarterclayton smarterclayton

@thockin thockin

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.