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

Enhancement for Git Repository Locations #45

Open
carlmes opened this issue Sep 25, 2024 · 2 comments
Open

Enhancement for Git Repository Locations #45

carlmes opened this issue Sep 25, 2024 · 2 comments

Comments

@carlmes
Copy link
Member

carlmes commented Sep 25, 2024

The Git repository locations are currently hard coded into the source yaml. This means that when contributing updates to this repo, either through a branch or a forked repository, the manual repo locations must be changed for testing, and then reverted prior to PR merging. Here's an example of this in a recent pull request:

  path: /spec/source/repoURL
  # TODO Change back when ready to merge back to upstream.
  # value: 'https://github.com/redhat-ai-services/ai-accelerator.git'
  value: 'https://github.com/fgharo/ai-accelerator.git'

To compound the problem, we now have multiple yaml files containing the comment "Update me on fork" with a hard coded URL, for example all the files in the cluster/overlays contain this logic with a hard coded Git repo location:

- op: replace
  path: /spec/source/repoURL
  value: 'https://github.com/redhat-ai-services/ai-accelerator.git'  # Update me on fork

Potential Enhancement #1

Use Kustomize environment variables so that there is a single location for the Git repository URL and branch name, this removes the requirement for multiple repository locations in multiple files to be updated during testing on a forked repository. It will also make it a lot easier to use this project in a disconnected environment, where GitHub is potentially unreachable and an alternate internal Git repo is used instead.

Steps to implement:

  • Create a Kustomize environment variables file
  • Change all of the currently hardcoded repo locations in the various overlays folders to use the environment configuration for Git repo and branch

Potential Enhancement #2

To solve the problem of merge requests containing accidental "TODO Change back when ready to merge" comments, we could make these Kustomize environment variables dynamically generated during the execution of bootstrap.sh.

It's suggested that the current Git repo (git remote -v) and branch (git branch) are dynamically added to a Kustomize environment file, such as environment-properties.env. To ensure that the user is aware of the Git repo and branch in use, the bootstrap script should be enhanced with a prompt such as:

Your environment will be provisioned through ArgoCD using the following Git repo, you can use default (press Enter) or change it:
- Git Repository [https://github.com/redhat-ai-services/ai-accelerator.git]: 
- Branch [main]: 

Where the defaults in the prompt are the Git remote origin and branch from the directory where the bootstrap script is being run.

Steps to implement:

  • Add Kustomize environment variables file to .gitignore since it will be dynamically generated
  • Enhance bootstrap script to obtain the Git defaults from the directory where it's being run
  • Add a prompt to alert the end user running the script of the location where ArgoCD will obtain configuration

Potential Enhancement #3

Another option is to use Git actions to verify and/or update the manually defined Git repo locations in the various YAML files. This has the advantage of keeping the code looking a little cleaner (no environment to worry about), but will require a little more maintenance in the long run to keep the list of files and locations up to date.

@strangiato
Copy link
Member

#46 is attempting to address enhancement option 3.

The goal of this is to provide a simple reminder to the user that they should be reverting those when they open a PR.

The script uses GitHub Actions annotation format to automatically annotate the line with the issue:

Screenshot 2024-09-26 at 3 49 37 PM

I think that there is still an opportunity to enhance the bootstrap process to automatically set some of these values when users are forking/branching.

@strangiato
Copy link
Member

For the env variable piece, the vars feature in kustomize that the linked article in # 2 uses is a depreciated feature that has been replaced with replacements

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

No branches or pull requests

2 participants