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.

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

allow exceptions to be specified to handle conflicting group and resource names #49224

Merged
merged 1 commit into from
Jul 20, 2017

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Jul 19, 2017

When a group name and resource name conflict, the generated code doesn't have prefixes or suffixes to produce compiling code. Instead, it simply produces code that won't compile.

This makes it possible for the code generator to have a special kind of namer that can codify the exceptions to get compiling code. As we move the generators to become more general, this should be updated to be plumbed by flags.

@gmarek give this a try in your event pull. Specify your type and see if the names are adjusted.
@sttts we hit this downstream

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 19, 2017
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Jul 19, 2017
Copy link
Member

@wojtek-t wojtek-t left a comment

Choose a reason for hiding this comment

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

Just super minor nits - feel free to self-lgtm.

}
privateNamer := &ExceptionNamer{
Exceptions: map[string]string{
// you can put your fully qualified package like
Copy link
Member

Choose a reason for hiding this comment

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

nit - can you make those comments exactly the same? Current the order of lines is slightly different.

if exception, ok := n.Exceptions[key]; ok {
return exception
}

Copy link
Member

Choose a reason for hiding this comment

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

nit: remove empty line

@wojtek-t
Copy link
Member

/approve


publicNamer := &ExceptionNamer{
Exceptions: map[string]string{
// these exceptions are used to deconflict the generated code
Copy link
Contributor

Choose a reason for hiding this comment

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

should we have flags for these?

@sttts
Copy link
Contributor

sttts commented Jul 20, 2017

As discusses in irc:

/lgtm

Follow-up after #49114 goes in: turn all constants in our generators into command line flags.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 20, 2017
@sttts sttts added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Jul 20, 2017
@sttts
Copy link
Contributor

sttts commented Jul 20, 2017

/approve no-issue

@deads2k
Copy link
Contributor Author

deads2k commented Jul 20, 2017

@sttts I will hold for your larger one to merge first. ping me when I'm good to rebase.

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 20, 2017
@deads2k
Copy link
Contributor Author

deads2k commented Jul 20, 2017

comments addressed.

@deads2k
Copy link
Contributor Author

deads2k commented Jul 20, 2017

/test pull-kubernetes-federation-e2e-gce

@wojtek-t
Copy link
Member

/lgtm
/approve no-issue

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, sttts, wojtek-t

Associated issue requirement bypassed by: wojtek-t

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 Jul 20, 2017
@deads2k deads2k added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Jul 20, 2017
@deads2k
Copy link
Contributor Author

deads2k commented Jul 20, 2017

holding for @sttts move

@deads2k deads2k removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Jul 20, 2017
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 20, 2017
@deads2k deads2k added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jul 20, 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 Jul 20, 2017
@deads2k
Copy link
Contributor Author

deads2k commented Jul 20, 2017

/retest

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 49107, 47177, 49234, 49224, 49227)

@k8s-github-robot k8s-github-robot merged commit d8ac3af into kubernetes:master Jul 20, 2017
@deads2k deads2k deleted the client-01-plurals branch August 3, 2017 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants