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

chore: add kubefed fork #1036

Merged
merged 2 commits into from
Mar 16, 2021
Merged

chore: add kubefed fork #1036

merged 2 commits into from
Mar 16, 2021

Conversation

hectorj2f
Copy link

Signed-off-by: Hector Fernandez [email protected]

What type of PR is this?

Chore
What this PR does/ why we need it:

While we are waiting to get kubernetes-retired/kubefed#1377 merged, we need to workaround that delay by getting our internal kubefed version based on v0.5.1.

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?:


Checklist

  • If a chart is changed, the chart version is correctly incremented.
  • The commit message explains the changes and why are needed.
  • The code builds and passes lint/style checks locally.
  • The relevant subset of integration tests pass locally.
  • The core changes are covered by tests.
  • The documentation is updated where needed.

Signed-off-by: Hector Fernandez <[email protected]>
@hectorj2f hectorj2f added the ready ready label Mar 16, 2021
@hectorj2f hectorj2f self-assigned this Mar 16, 2021
@hectorj2f hectorj2f requested a review from a team as a code owner March 16, 2021 16:37
@jimmidyson
Copy link
Contributor

We can get kubernetes-retired/kubefed#1377 merged quickly (like tomorrow morning) I'm sure. Better than using a fork here imo.

Comment on lines +395 to +397
proxyURL:
description: ProxyURL allows to set proxy URL for the cluster.
type: string
Copy link
Contributor

Choose a reason for hiding this comment

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

This field is the reason we're adding fork of kubefed chart.

Copy link
Contributor

@mhrabovcin mhrabovcin left a comment

Choose a reason for hiding this comment

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

@mhrabovcin
Copy link
Contributor

@jimmidyson the upstream PR is against master branch. As @hectorj2f mentioned yakcl upgrade of kubefed would be non-trivial and it is not in the scope of our unidirectional work.

With merging this PR we'll get unblocked by temporarily using forked kubefed chart + custom built kubefed v0.5.2 containers. Once we have confidence in delivering unidirectional feature on time we'll invest time in upstream PR and bumping yakcl to use latest kubefed.

Copy link
Contributor

@joejulian joejulian left a comment

Choose a reason for hiding this comment

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

I see that this is only a temporary fork so I'll approve despite my CR.

@joejulian joejulian merged commit 688e7c5 into master Mar 16, 2021
Copy link
Contributor

@gracedo gracedo left a comment

Choose a reason for hiding this comment

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

This is only pulled in for yakcl right? Just wondering if it's easier to pull the chart directly into the kommander-federation local charts dir instead of using an extra m/charts fork, esp if this is just temporary

@joejulian joejulian deleted the hectorj2f/bring_kubefed_fork branch March 16, 2021 19:04
d2iq-dispatch added a commit that referenced this pull request Mar 16, 2021
* chore: add kubefed fork

Signed-off-by: Hector Fernandez <[email protected]>

* chore: update test to exclude our own kubefed fork chart

Signed-off-by: Hector Fernandez <[email protected]>
@mhrabovcin
Copy link
Contributor

@gracedo I don't think we've considered this option. But you are right, this is temporary solution and the chart will be removed from our charts repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants