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

Refactor GitOps approach prompt flow in guided init #2269

Merged
merged 14 commits into from
Sep 5, 2024

Conversation

marcelovilla
Copy link
Member

@marcelovilla marcelovilla commented Feb 21, 2024

Reference Issues or PRs

Closes #2268

What does this implement/fix?

Put a x in the boxes that apply

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds a feature)
  • Breaking change (fix or feature that would cause existing features not to work as expected)
  • Documentation Update
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build related changes
  • Other (please describe):

Testing

  • Did you test the pull request locally?
  • Did you add new tests?

Any other comments?

How to test this PR

While we have unit tests covering the functionality changes in this PR, I present three different paths a reviewer might want to test manually.

Choose GitLab during the GitOps approach prompts

  1. Run nebari init --guided-init
  2. Follow through with any options and select y when prompted with Would you like to adopt a GitOps approach to managing Nebari? (y/N)
  3. Choose gitlab.com

You should be prompted with further configuration options. Follow through selecting N for the following questions.

  1. Run nebari render -c nebari-config.yaml

You should see a .gitlab-ci.yml file.

Choose GitHub during the GitOps approach prompts

  1. Run nebari init --guided-init
  2. Follow through with any options and select y when prompted with Would you like to adopt a GitOps approach to managing Nebari? (y/N)
  3. Choose github.com
  4. Choose N when prompted with Would you like nebari to create a remote repository on github.com?

You should be prompted with further configuration options. Follow through selecting N for the following questions.

  1. Run nebari render -c nebari-config.yaml

You should see a .github/workflows folder.

Choose GitHub with auto-provisioning during the GitOps approach prompts

  1. Run nebari init --guided-init
  2. Follow through with any options and select y when prompted with Would you like to adopt a GitOps approach to managing Nebari? (y/N)
  3. Choose github.com
  4. Choose y when prompted with Would you like nebari to create a remote repository on github.com?
  5. Pass an organization you have access to
  6. Pass a repo name to be created
  7. Paste your github username and token if necessary

You should be prompted with further configuration options. Follow through selecting N for the following questions.

  1. Run nebari render -c nebari-config.yaml

You should see a .github/workflows folder and there should be newly created GitHub repo under the organization you specified.

@marcelovilla
Copy link
Member Author

Unit tests are failing. I'm looking into it.

Copy link
Member

@aktech aktech left a comment

Choose a reason for hiding this comment

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

I have little suggestion for improving the regex. I would also suggest writing some tests, like for example:

import re
import pytest

@pytest.mark.parametrize("url", [
    "https://github.com/user/repository",
    "https://gitlab.com/user/repository/",
])
def test_valid_git_repo_urls(url):
    assert re.match(git_repo_url_regex, url) is not None, f"Failed to match {url}"

@pytest.mark.parametrize("url", [
    "https://github.com",
    "https://github.com/user",
    "https://gitlab.com/user",
    "http://github.com/user/",
    "gitlab.com/user/",
    "github.com",
])
def test_invalid_git_repo_urls(url):
    assert re.match(git_repo_url_regex, url) is None, f"Unexpected match for {url}"

src/nebari/schema.py Outdated Show resolved Hide resolved
@marcelovilla
Copy link
Member Author

@aktech I modified the existing tests and added new ones based on your suggestion. I should have added new tests in the first place so thanks for bringing that up!

src/nebari/schema.py Outdated Show resolved Hide resolved
@dcmcand
Copy link
Contributor

dcmcand commented Mar 18, 2024

@marcelovilla can you update the status here?

@dcmcand
Copy link
Contributor

dcmcand commented Jun 18, 2024

@marcelovilla what is the status of this PR? Are you still working on it?

@marcelovilla marcelovilla changed the title Modify regular expression to match both github and gitlab repos. Refactor GitOps approach prompt flow in guided init Aug 13, 2024
@marcelovilla
Copy link
Member Author

This PR was stale for a very long time but I've just added some changes to get it across the finish line. You'll see that I decided to revert the regex changes I wrote before (and thus, also decided to change the title of the PR). The reason behind this is that I did not see any point in modifying the regex to allow both GitHub and GitLab repos when we don't really support repo auto-provisioning for GitLab.

I decided to keep only the regex for GitHub repo validation and instead change the flow in which we present the user with the GitOps approach prompts during the guided init. Previously, we were prompting the user for the following inputs:

  1. Provider (i.e., GitHub or GitLab)
  2. Organization and repository
  3. Whether they want Nebari to auto-provision the repository

When users selected GitLab for (1) and then gave an org and repo name for (2), they would get a validation error because of the repo URL (see #2268). While having a regex that validated both provider repos would have solved that issue, it wouldn't really have done anything with that information afterwards, because, as I mentioned before, we have not implemented the auto-provisioning logic for GitLab. I think implementing that is outside of the scope of this PR and will require a considerably larger effort to achieve. Furthermore, in the case where users choose GitHub as the provider, we only use the organization and repository name if they want Nebari to auto-provision the repository. Thus, with this PR my proposal is to modify the GitOps approach flow in the guided init to prompt the user for the following inputs:

  1. Provider (i.e., GitHub or GitLab)
  2. (Only if the provider is GitHub) whether they want Nebari to auto-provision the repository
  3. (Only if they want to auto-provision the repo) organization and repository

Additionally, I modified our render tests to make sure we're passing GitLab as a provider in one of the configurations. And unrelated to this, I fixed the original regex to avoid matching an edge case, which I added to the existing tests.

The PR description has a section on how to test this PR.

@Adam-D-Lewis Adam-D-Lewis self-requested a review August 27, 2024 20:47
Copy link
Member

@Adam-D-Lewis Adam-D-Lewis left a comment

Choose a reason for hiding this comment

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

LGTM

@marcelovilla
Copy link
Member Author

Thanks for the review @Adam-D-Lewis

@aktech I just realized I cannot merge this as you requested changes a while ago. Can you have a look and see what you think?

@marcelovilla
Copy link
Member Author

@aktech just wanted to give this PR a nudge when you have some time. Thanks!

Copy link
Member

@aktech aktech left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

@aktech aktech merged commit 0162d3d into develop Sep 5, 2024
29 checks passed
@aktech aktech deleted the bugfix/gitlab-setup branch September 5, 2024 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done 💪🏾
Development

Successfully merging this pull request may close these issues.

[BUG] - Nebari init fails when using a GitLab repo for the GitOps approach
5 participants