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

Consistent Names for ControllerRevisions, ReplicaSets, and objects using GenerateName #51538

Merged
merged 1 commit into from
Sep 3, 2017
Merged

Consistent Names for ControllerRevisions, ReplicaSets, and objects using GenerateName #51538

merged 1 commit into from
Sep 3, 2017

Conversation

kow3ns
Copy link
Member

@kow3ns kow3ns commented Aug 29, 2017

What this PR does / why we need it:
Adds the rand.SafeEncodeString function and uses this function to generate names for ReplicaSets and ControllerRevisions.

The names generated for ControllerRevision and ReplicaSet are consistent with the GenerateName functionality of the API Server and will not contain "bad words".

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 29, 2017
@kow3ns kow3ns added this to the v1.8 milestone Aug 29, 2017
@k8s-github-robot k8s-github-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Aug 29, 2017
@kow3ns kow3ns requested review from janetkuo and 0xmichalis August 29, 2017 16:52
@kow3ns
Copy link
Member Author

kow3ns commented Aug 29, 2017

/retest

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 30, 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 Aug 30, 2017
@janetkuo janetkuo self-assigned this Aug 30, 2017
@kow3ns
Copy link
Member Author

kow3ns commented Aug 30, 2017

/retest

Adds the rand.SafeEncodeString function and uses this function to
313a8b3 
generate names for ReplicaSets and ControllerRevisions.
janetkuo
janetkuo reviewed Aug 30, 2017
View reviewed changes
staging/src/k8s.io/apimachinery/pkg/util/rand/rand.go

// SafeEncodeString encodes s using the same characters as rand.String. This reduces the chances of bad words and
// ensures that strings generated from hash functions appear consistent throughout the API.
func SafeEncodeString(s string) string {
Copy link
Member

Choose a reason for hiding this comment

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

Suggest putting this function elsewhere since the returned value isn't random

Copy link
Member Author

Choose a reason for hiding this comment

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

It depends on the slice used random names generated by String() string in the same package. Its use is also closely tied to this function. It may warrant refactoring this code, but I'd rather focus on ensuring that the Names contain no bad words in this PR and a refactor in a followup.

Copy link
Member

Choose a reason for hiding this comment

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

ok!

@janetkuo
Copy link
Member

/retest

3 similar comments
@kow3ns
Copy link
Member Author

kow3ns commented Aug 31, 2017

/retest

@kow3ns
Copy link
Member Author

kow3ns commented Aug 31, 2017

/retest

@kow3ns
Copy link
Member Author

kow3ns commented Aug 31, 2017

/retest

@0xmichalis
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 31, 2017
@deads2k
Copy link
Contributor

deads2k commented Aug 31, 2017

/approve no-issue

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, kargakis, kow3ns

Associated issue requirement bypassed by: deads2k

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 /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 31, 2017
@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 51553, 51538, 51663, 51069, 51737)

@k8s-github-robot k8s-github-robot merged commit 52e3fda into kubernetes:master Sep 3, 2017
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Sep 3, 2017 •
edited
Loading

@kow3ns: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-kops-aws 313a8b3 link /test pull-kubernetes-e2e-kops-aws

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.

@jdumars
Copy link
Member

jdumars commented Sep 6, 2017

Could you please assign a SIG to this PR please? @kow3ns

@andor44 andor44 mentioned this pull request Oct 31, 2017
Closed
@abstrctn abstrctn mentioned this pull request Nov 8, 2017
Closed
@islinwb islinwb mentioned this pull request Mar 5, 2018
Closed
@mrbobbytables mrbobbytables mentioned this pull request Jun 29, 2018
Closed
@bendrucker bendrucker mentioned this pull request Sep 20, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@janetkuo janetkuo janetkuo approved these changes

@0xmichalis 0xmichalis Awaiting requested review from 0xmichalis

Assignees

@0xmichalis 0xmichalis

@janetkuo janetkuo

@derekwaynecarr derekwaynecarr

@deads2k deads2k

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. lgtm "Looks good to me", 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/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
v1.8
Development

Successfully merging this pull request may close these issues.
8 participants
@kow3ns @janetkuo @0xmichalis @deads2k @k8s-github-robot @k8s-ci-robot @jdumars @derekwaynecarr