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

Follow on to the AuthorizationAttributes -> Action PR to fix authorization conversions #9838

Closed
wants to merge 3 commits into from
Closed

Follow on to the AuthorizationAttributes -> Action PR to fix authorization conversions #9838

wants to merge 3 commits into from

Conversation

smarterclayton
Copy link
Contributor

Also lays groundwork for protobuf

[test] @deads2k

@smarterclayton smarterclayton mentioned this pull request Jul 14, 2016
Closed
@smarterclayton smarterclayton force-pushed the fix_conversions branch 2 times, most recently from c5fc288 to ffdbff0 Compare July 14, 2016 04:59
deads2k
deads2k reviewed Jul 14, 2016
View reviewed changes
api/swagger-spec/oapi-v1.json
@@ -1,7 +1,7 @@
{
"swaggerVersion": "1.2",
"apiVersion": "v1",
"basePath": "https://127.0.0.1:8443",
"basePath": "https://127.0.0.1:38443",
Copy link
Contributor

Choose a reason for hiding this comment

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

odd

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

@deads2k
Copy link
Contributor

deads2k commented Jul 14, 2016

I really dislike anonymous fields. I had to do it externally for backwards compatibility, but internally it just makes a mess to have them anonymous.

@smarterclayton
Copy link
Contributor Author

I think the internal fields can have names, but probably won't get to it in
the near term. Will leave a follow up to do that.

On Thu, Jul 14, 2016 at 10:23 AM, David Eads notifications@github.com
wrote:

I really dislike anonymous fields. I had to do it externally for backwards
compatibility, but internally it just makes a mess to have them anonymous.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#9838 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABG_p6JPbwxNHJaRn0OggjQa_i_asSY8ks5qVkZmgaJpZM4JMCT8
.

@smarterclayton
UPSTREAM: 28931: genconversion=false should skip fields during conver…
1fa99a3 
…sion generation

Currently it only skips if the fields don't match, but that leaves no
way for callers to say "no really, ignore this field".
@smarterclayton
Rename AuthorizationAttributes -> Action
9c482b5 
Allows conversions to be automatically generated, and the name is not
externally visible. Subsequent commits will update the conversions.
@smarterclayton
Make authorization conversions efficient
d25c78f 
Prepare for protobuf by adding aliases for nullable fields.
@openshift-bot
Copy link
Contributor

Evaluated for origin test up to d25c78f

@openshift-bot
Copy link
Contributor

Origin Action Required: Pull request cannot be automatically merged, please rebase your branch from latest HEAD and push again

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 19, 2016
@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/6425/)

@smarterclayton
Copy link
Contributor Author

Moving into #9814 because the more extensive round trip testing and validation going on there. All comments here answered or addressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@smarterclayton @deads2k @openshift-bot