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

Rename EnvVarSource.FieldPath -> FieldRef and add example #7592

Merged
merged 1 commit into from May 5, 2015

Conversation

Projects
None yet
4 participants
@pmorie
Copy link
Member

pmorie commented Apr 30, 2015

No description provided.

@googlebot

This comment has been minimized.

Copy link

googlebot commented Apr 30, 2015

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@googlebot googlebot added the cla: no label Apr 30, 2015

@pmorie

This comment has been minimized.

Copy link
Member Author

pmorie commented Apr 30, 2015

@smarterclayton

This comment has been minimized.

Copy link
Contributor

smarterclayton commented May 1, 2015

fieldPath: fieldPath? Really?

@pmorie

This comment has been minimized.

Copy link
Member Author

pmorie commented May 1, 2015

@smarterclayton Really.

@smarterclayton

This comment has been minimized.

Copy link
Contributor

smarterclayton commented May 1, 2015

Ugh.... wasn't that supposed to be reference fieldPath?

@pmorie

This comment has been minimized.

Copy link
Member Author

pmorie commented May 1, 2015

@smarterclayton I had thought @bgrant0607 explicitly wanted fieldPath for both. I'll go back and read through the comments again.

@pmorie

This comment has been minimized.

Copy link
Member Author

pmorie commented May 1, 2015

@smarterclayton

This comment has been minimized.

Copy link
Contributor

smarterclayton commented May 1, 2015

Hrm, I think seeing it in the actual API makes me think it was a bad decision. We can revisit later but I'm not a fan of redundant redundancy.

----- Original Message -----

@smarterclayton
#6739 (comment)


Reply to this email directly or view it on GitHub:
#7592 (comment)

@pmorie

This comment has been minimized.

Copy link
Member Author

pmorie commented May 1, 2015

@smarterclayton Yeah, I felt the same thing -- I'm trying to think of better names but coming up empty.

@j3ffml j3ffml assigned smarterclayton and unassigned bgrant0607 May 1, 2015

@bgrant0607

This comment has been minimized.

Copy link
Member

bgrant0607 commented May 2, 2015

ObjectFieldSelector.FieldPath needs to stay FieldPath in order to match ObjectReference.

Other possibilities for EnvVarSource.FieldPath:

  • Field
  • FieldRef
  • ObjectFieldRef
@bgrant0607

This comment has been minimized.

Copy link
Member

bgrant0607 commented May 2, 2015

FieldRef makes the most sense to me.

@smarterclayton

This comment has been minimized.

Copy link
Contributor

smarterclayton commented May 2, 2015

Agreed

On May 1, 2015, at 8:26 PM, Brian Grant notifications@github.com wrote:

FieldRef makes the most sense to me.


Reply to this email directly or view it on GitHub.

@pmorie

This comment has been minimized.

Copy link
Member Author

pmorie commented May 2, 2015

Alrighty, I will change it.
On Fri, May 1, 2015 at 8:29 PM Clayton Coleman notifications@github.com
wrote:

Agreed

On May 1, 2015, at 8:26 PM, Brian Grant notifications@github.com
wrote:

FieldRef makes the most sense to me.


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHub
#7592 (comment)
.

@pmorie pmorie changed the title Add downward API example Rename EnvVarSource.FieldPath -> FieldRef and add example May 4, 2015

@pmorie pmorie force-pushed the pmorie:dapi-example branch from e3ee435 to 324992f May 4, 2015

@pmorie

This comment has been minimized.

Copy link
Member Author

pmorie commented May 4, 2015

@smarterclayton updated, PTAL

@pmorie pmorie force-pushed the pmorie:dapi-example branch from 324992f to 772d677 May 4, 2015

@smarterclayton

This comment has been minimized.

Copy link
Contributor

smarterclayton commented May 4, 2015

LGTM, may need to regenerate docs as well (put them in their own commit).

@pmorie

This comment has been minimized.

Copy link
Member Author

pmorie commented May 4, 2015

@smarterclayton I got no real diffs from regenerating

@smarterclayton

This comment has been minimized.

Copy link
Contributor

smarterclayton commented May 4, 2015

Sorry, I meant swagger, not docs.

@pmorie

This comment has been minimized.

Copy link
Member Author

pmorie commented May 4, 2015

@smarterclayton It's a lot of changes for the swagger spec -- do you want it in this PR?

@smarterclayton

This comment has been minimized.

Copy link
Contributor

smarterclayton commented May 4, 2015

Separate commit.

@bgrant0607

This comment has been minimized.

Copy link
Member

bgrant0607 commented May 4, 2015

LGTM

@smarterclayton

This comment has been minimized.

Copy link
Contributor

smarterclayton commented May 4, 2015

Will merge on green travis

@bgrant0607

This comment has been minimized.

Copy link
Member

bgrant0607 commented May 4, 2015

I recommend postponing the swagger updates to a separate PR. Unrelated changes are causing the validation test to fail when the swagger is updated.

@pmorie

This comment has been minimized.

Copy link
Member Author

pmorie commented May 4, 2015

@bgrant0607 fine with me.

On Mon, May 4, 2015 at 6:00 PM, Brian Grant notifications@github.com
wrote:

I recommend postponing the swagger updates to a separate PR. Unrelated
changes are causing the validation test to fail when the swagger is updated.


Reply to this email directly or view it on GitHub
#7592 (comment)
.

@smarterclayton

This comment has been minimized.

Copy link
Contributor

smarterclayton commented May 4, 2015

It doesn't matter because travis will never give you a green

@bgrant0607

This comment has been minimized.

Copy link
Member

bgrant0607 commented May 4, 2015

What do you mean @smarterclayton? Shippable failed on this PR, apparently due to the swagger update.

cc @nikhiljindal

@smarterclayton

This comment has been minimized.

Copy link
Contributor

smarterclayton commented May 4, 2015

It's been waiting for a good 4-5 hours to start

@bgrant0607

This comment has been minimized.

Copy link
Member

bgrant0607 commented May 4, 2015

Also, the generated swagger still contains fieldPath rather than fieldRef.

@pmorie pmorie force-pushed the pmorie:dapi-example branch from 2d799e4 to e949a62 May 4, 2015

@googlebot

This comment has been minimized.

Copy link

googlebot commented May 4, 2015

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels May 4, 2015

@pmorie

This comment has been minimized.

Copy link
Member Author

pmorie commented May 4, 2015

@bgrant0607 Removed the swagger commit, I will look into that separately as a follow-up.

@bgrant0607

This comment has been minimized.

Copy link
Member

bgrant0607 commented May 4, 2015

LGTM

@bgrant0607

This comment has been minimized.

Copy link
Member

bgrant0607 commented May 4, 2015

It looks like we need to update go-restful again to get the *int64 fix:
emicklei/go-restful@63c8f08

@bgrant0607

This comment has been minimized.

Copy link
Member

bgrant0607 commented May 4, 2015

Rerunning Shippable.

bgrant0607 added a commit that referenced this pull request May 5, 2015

Merge pull request #7592 from pmorie/dapi-example
Rename EnvVarSource.FieldPath -> FieldRef and add example

@bgrant0607 bgrant0607 merged commit df8521c into kubernetes:master May 5, 2015

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Shippable Shippable builds completed
Details
cla/google All necessary CLAs are signed

@pmorie pmorie referenced this pull request May 5, 2015

Merged

Fix swagger spec #7779

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.