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

make generate about to break when kustomize new release is published. #419

Merged
merged 3 commits into from
Oct 6, 2019

Conversation

jbrette
Copy link
Contributor

@jbrette jbrette commented Oct 2, 2019

Which issue is resolved by this Pull Request:
Resolves #
Kustomize is about to change its interfaces, and this will breaks the "make generate" and "make test"

  • Changes in the fs packages.
  • Internalization of the build command.
  • Various cleanups.

Description of your changes:
Just add the go.mod and remove it from the .gitignore. This will force make generate and make test to use a version of kustomize compatible with kubeflow instead of letting it pickup the latest published version of kustomize.

Another PR will have to be created to adapt the code to the new version of kustomize once it is released.

Checklist:

  • Unit tests have been rebuilt:
    1. cd manifests/tests
    2. make generate
    3. make test

This change is Reviewable

@k8s-ci-robot
Copy link
Contributor

Hi @jbrette. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Copy link
Contributor

@kkasravi kkasravi left a comment

Choose a reason for hiding this comment

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

Jerome

could you also delete

go get sigs.k8s.io/kustomize/v3

in tests/scripts/run-tests.sh?
This repo uses test-worker for prow presubmit which is already using KUSTOMIZE=v3.2.0

go.mod Show resolved Hide resolved
@kkasravi
Copy link
Contributor

kkasravi commented Oct 3, 2019

/ok-to-test

@jbrette
Copy link
Contributor Author

jbrette commented Oct 4, 2019

@kkasravi

  • Done requested code review.
  • Tested inside and outside GOPATH.
  • Added test.go that where missing after yaml was changed with rerunning generation

/assign @jlewi

@kkasravi
Copy link
Contributor

kkasravi commented Oct 5, 2019

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kkasravi

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

@kkasravi
Copy link
Contributor

kkasravi commented Oct 5, 2019

@jbrette could i ask you to rebase from master?
git fetch upstream master
git rebase -i upstream/master

@jbrette
Copy link
Contributor Author

jbrette commented Oct 5, 2019

@jbrette could i ask you to rebase from master?
git fetch upstream master
git rebase -i upstream/master

done. reran make generated and git push --force. You will have to reapply labels

@@ -49,7 +49,7 @@ spec:
- jupyter
- notebook
- notebook-controller
- jupyterhub
- jupyterhub
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we still have jupyterhub here? That package no longer exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is why: https://github.com/kubeflow/manifests/blob/master/jupyter/notebook-controller/overlays/application/application.yaml#L35

@jlewi @kkasravi I real don't want to be in the same situation than we you asked to remove Tiller and Heritage at the same time people were reverting changes to go back to deprecated version of Ingress, boggus changes which are still here by way as as the Tiller stuff.

So tell me what you want....I can drop the "generation code" commit from this PR so that you can merge...and one of you will have to run make generate again later

Copy link
Contributor

Choose a reason for hiding this comment

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

it is unrelated to what you're trying to get merged.
It's ok with me if we merge this and then remove jupyter in a subsequent PR.
@jlewi ?

/lgtm

@k8s-ci-robot k8s-ci-robot merged commit ea58d15 into kubeflow:master Oct 6, 2019
@jbrette jbrette deleted the gomodpinning branch October 6, 2019 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants