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

added support for registration flow env variable #1335

Merged
merged 1 commit into from
Jul 7, 2020
Merged

added support for registration flow env variable #1335

merged 1 commit into from
Jul 7, 2020

Conversation

maganaluis
Copy link
Contributor

Which issue is resolved by this Pull Request:
Resolves # kubeflow/kubeflow#4889

Description of your changes:

This addresses the support needed to disable the registration flow on issue 4889.

I modified the deployment patch, base kustomization and params.env for /common/centraldashboard to add another environment variable called REGISTRATION_FLOW which by default is set to "true". If the user wants to disable the registration flow they have to modify the definitions for the central-dashboard, add the registration-flow variable and set it to "false".

  - kustomizeConfig:
      overlays:
      - istio
      - application
      parameters:
      - name: userid-header
        value: kubeflow-userid
      - name: registration-flow
        value: "false"
      repoRef:
        name: manifests
        path: common/centraldashboard
    name: centraldashboard

Checklist:

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

@kubeflow-bot
Copy link
Contributor

This change is Reviewable

@k8s-ci-robot
Copy link
Contributor

Hi @maganaluis. 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.

@maganaluis
Copy link
Contributor Author

@lalithvaka This PR should address 4889, the documentation should also be pretty straight forward.

@krishnadurai
Copy link
Contributor

/ok-to-test

@krishnadurai
Copy link
Contributor

/assign

@maganaluis
Copy link
Contributor Author

@krishnadurai one of the tests failed, is there documentation to run them locally?

@krishnadurai
Copy link
Contributor

According to this link it seems that you would have to generate tests by running make generate from within tests folder.

Run make test to verify. Commit the generated unit-tests upstream.

@krishnadurai
Copy link
Contributor

@maganaluis
Copy link
Contributor Author

/retest

@maganaluis
Copy link
Contributor Author

maganaluis commented Jun 30, 2020

@krishnadurai Thank you for the information, looks like the tests passed, the bot now seems to be waiting for the approval label.

Copy link
Contributor

@krishnadurai krishnadurai left a comment

Choose a reason for hiding this comment

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

/lgtm

@krishnadurai
Copy link
Contributor

@jlewi can you please approve?

@jlewi
Copy link
Contributor

jlewi commented Jul 7, 2020

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jlewi

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

@jlewi
Copy link
Contributor

jlewi commented Jul 7, 2020

If you want this to be in 1.1 you will need to cherry pick it onto the 1.1 branch.

@lalithvaka
Copy link

@maganaluis , it appears that you may need to update the image tag to get this pick up your latest changes from kubeflow code

@maganaluis
Copy link
Contributor Author

@lalithvaka actually because of the switch to kustomize v3 to build the Kubeflow manifests, setting this env variable will vary between platforms. I verified the change is in the 1.1 image v1.1.0-g35d7484a

@lalithvaka
Copy link

@maganaluis , does that mean you do not have to explicitly update the image tag in your PR? And we can move forward with the cherry pick pull that I submitted?

@maganaluis
Copy link
Contributor Author

@lalithvaka Yes the cherry pick can be merged, but configuring the env variable is up to the platform owner. See the new pattern here https://github.com/kubeflow/manifests/tree/master/stacks

lalithvaka added a commit to lalithvaka/manifests that referenced this pull request Jul 23, 2020
maganaluis added a commit to lalithvaka/manifests that referenced this pull request Jul 23, 2020
k8s-ci-robot pushed a commit that referenced this pull request Jul 24, 2020
…nv variable Cherry pick of #1335 on v1.1-branch. #1335: added support for registration flow env variable (#1370)

* added support for registration flow env variable

adding expected test changes

* Revert "Merge branch 'v1.1-branch' into automated-cherry-pick-of-#1335-upstream-v1.1-branch"

This reverts commit cfe177f.

Co-authored-by: Luis <maganaluis92@gmail.com>
@pauljegouic
Copy link

tried to change value from true to false in common/centraldashboard/base/params.env, but there is no change on KF.

  • it seems that the container config has not been successfully applied.

Need help please, what I am doing wrong ?

@pauljegouic
Copy link

@krishnadurai

@maganaluis
Copy link
Contributor Author

@ifs-pauljegouic Feel free to post your issue on Slack, or ping me there. Check that you have the 1.1 version of the central dashboard image, as well as the registration-flow env variable on the Deployment set to "false".

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.

8 participants