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

Upgrade refs to use 1.11/8.0 #32

Merged

Conversation

CharlyF
Copy link
Contributor

@CharlyF CharlyF commented Sep 26, 2018

Hey!
First off, thanks a lot for all the work you do, I'm using extensively the custom-metrics repo at my company and it's awesome!

As you probably know, some configs allow the creation of a requestheader CA in the extension-apiserver-authentication ConfigMap but no client CA (in EKS notably). This has been an issue for the metrics server which you fixed in 0.3.0 kubernetes/apiserver@e59fba7

But the commit ref in the lock for the apiserver here is too old, so the upstream fix is not embedded.

I don't believe that this contribution will be merged, but I wanted to bring it up to your attention.
I have been using this code on my external metrics server successfully - If you find time to upgrade the glide here, that would be fantastic. Hopefully this PR might help!

Thanks again,
Charly

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 26, 2018
@DirectXMan12
Copy link
Contributor

thanks for the PR! I'm in the process of updating some other repos with similar changes right now, so I want to double-check that the dependency versions listed here don't cause vendoring issues, but it they don't, I'll merge this.

@DirectXMan12 DirectXMan12 changed the title Upgrade refs to use 1.11/0.8 Upgrade refs to use 1.11/8.0 Oct 3, 2018
@DirectXMan12
Copy link
Contributor

DirectXMan12 commented Oct 3, 2018

Works for me. Hmm... looks like travis is trying to run glide, which isn't in Travis. Can you make sure glide.lock and vendor are up-to-date?

@CharlyF
Copy link
Contributor Author

CharlyF commented Oct 3, 2018

Works for me. Hmm... looks like travis is trying to run glide, which isn't in Travis. Can you make sure glide.lock and vendor are up-to-date?

The hash is pretty consistent but I've tried a few things, locally it works fine - I wonder if it could be the cache on Travis's side ?

@mfpierre
Copy link

mfpierre commented Oct 4, 2018

@DirectXMan12 I think there's an issue with the current Makefile

vendor: glide.lock will run glide install is the glide.lock has been modified after the vendor directory, the thing is you can't really control this date with git. (because they are not PHONY targets)

State when I clone the repo:

-rw-r--r--   1 pierremargueritte  staff  16583 Oct  4 14:59 glide.lock
drwxr-xr-x   8 pierremargueritte  staff    256 Oct  4 14:54 vendor

so make vendor will run glide install

but if I do touch vendor then make: "vendor" is up to date.

We have several solutions:

  • add glide back to travis
  • remove vendoras a prerequisite for test, and maybe add another target for this

Not sure what strategy do you want for this repo?

@DirectXMan12
Copy link
Contributor

I'd remove vendor as a dependency for everything, it gets weird otherwise.

@CharlyF
Copy link
Contributor Author

CharlyF commented Oct 4, 2018

Let me know if that looks better @DirectXMan12. Thanks for your review 🙇

@CharlyF
Copy link
Contributor Author

CharlyF commented Oct 5, 2018

I just saw that #34 got submitted - It will be great to have 1.12 in the incubator!
Is there any chance we can have this PR merged before though, so the change is in the commit history ? I'd would like to pin 1.11 with the upstream fix in my implementation of the External Metrics Provider instead of having to port all the interfaces to comply with 1.12. This is a project I've planned on tackling but later this month.

Let me know your thoughts 🙂

@astefanutti
Copy link
Member

@CharlyF I only noticed this PR after I created #34. Agreed both PRs can be merged sequentially so we're on the safe side.

@CharlyF
Copy link
Contributor Author

CharlyF commented Oct 23, 2018

@DirectXMan12, sorry for the direct ping - Any chance you can let us know if this is good to go ?

@DirectXMan12
Copy link
Contributor

hey, sorry about the delay -- I was out of the office for the past few weeks. I'll try to review this this week, and hopefully get it merged.

@DirectXMan12
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 12, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DirectXMan12

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 12, 2018
@k8s-ci-robot k8s-ci-robot merged commit 85ebc28 into kubernetes-sigs:master Nov 12, 2018
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. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants