-
Notifications
You must be signed in to change notification settings - Fork 153
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
Implement add profile
to install a profile to a cluster
#1360
Conversation
c87b11d
to
aaaa875
Compare
add profile
to install a profile to a clusteradd profile
to install a profile to a cluster
30c6604
to
728b4b8
Compare
cmd/gitops/add/profiles/cmd.go
Outdated
func AddCommand(client *resty.Client) *cobra.Command { | ||
cmd := &cobra.Command{ | ||
Use: "profile", | ||
Aliases: []string{"profiles"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we want the alias since you can only install 1 at a time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | ||
} | ||
|
||
func validateAddOptions(opts profiles.AddOptions) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sets up an unfortunate error pattern, where you run it and it tells you something is invalid, so you fix it, and then you run it again, and it tells you something else is invalid.
Maybe a multierror
would be better?
At least, return a slice of strings with all the errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the problem that these depend on each other? If opts.Name
is not defined, you can't call ApplicationNameTooLong
. And then you can't call HasPrefix either. Though I agree that ConfigRepo and Cluster could be checked separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed this in the commit that addresses review feedback - the validateAddOptions
func only has 2 checks in it now since I moved the required flags to Cobra's feature for checking for required flags.
- MergePullRequest uses go-git-provider to merge a pull request from a repository, given that repo's URL and the PR number.
- Improve acceptance test for Profiles service - Improve logic for creating kind clusters to avoid duplicate prefix e.g. kind-kind-x
e10b6a3
to
a799dec
Compare
- removed all `Expect(err).NotTo(BeNil)` before `MatchError()` assertions - refactored how we discover version of available profiles to return it rather than set it on pb profile - pass in all values for MakeHelmRelease to remove Profiles dependency - renamed --port to --profiles-port - use a single const for profiles path (git.ProfilesManifestFileName) instead of models.WegoProfilePath - seed with time at the start of the cmd for late use of rand - improve function name and documentation comment for splitYAML() and GetRandomString() - use Cobra's MarkFlagRequired feature for name, cluster, and config-repo flags - change gitproviders.MergePullRequest() to always use method Merge - uses ConvertStringListToSemanticVersionList() and SortVersions() to sort semver versions of Profile's available versions - checkout package-lock.json to reset it to that of main - removed *resty.Client from addRunE command - moved profiles.yaml const to pkg models - added --kubeconfig flag - renamed GetRepoFiles() to GetRepoDirFiles() - renamed MakeManifestFile() to AppendProfileToFile() - renamed GetAvailableProfile() to GetProfile() - MakeHelmRelease compares HelmReleases with go-cmp (very cool) Co-authored-by: Jake Klein <[email protected]> Co-authored-by: Gergely Brautigam <[email protected]> Co-authored-by: Chetan Patwal <[email protected]>
a799dec
to
80fd25f
Compare
Hey folks 👋 @aclevername @Skarlso @cPu1 @Himangini @bigkevmcd @jpellizzari Thank you all for the reviews! I've responded to and/or addressed all feedback. I didn't want to spam everyone by responding with Feel free to mark any leftover comment as resolved and/or review and comment more! PS: integration tests + linter currently pass ✅ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done Niki 👏🏻 , I have gone through all the files and have a few minor comments, otherwise the PR looks pretty good to me 🎉
For Documentation, adding something under https://docs.gitops.weave.works/docs/cluster-management/profiles section should be sufficient I think. cc @sympatheticmoose @josefaworks wdyt? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with that one last break
addition. :) And Kevin's comment about the possible too long name.
4f77b16
to
b40f6f7
Compare
b40f6f7
to
011bfdd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 👍🏻
Closes: #1109
What changed? Why?
This PR adds the ability to install a profile to a cluster.
Usage
Minimum/required flags:
gitops add profile --name=podinfo --cluster=prod --config-repo=ssh://[email protected]/owner/config-repo.git
Optional flags:
--namespace
defaults towego-system
--version
defaults tolatest
--port
for the Profiles API port--auto-merge
--kubeconfig
Implementation Notes
HelmRepository
Flux resource that contains the Chart must already be present and available for installation in the cluster.add profile
discovers theHelmRepository
's name and namespace through aGet
query to the Profiles API. While this PR tries to rely on the Git provider API to use the.weave-gitops
directory as the Source of Truth,add profile
defers toget profiles
to discover theHelmRepository
's name and namespace, and its workflow still requires access to the cluster through a local kubeconfig. However, this will become optional as part of the following ticket: Add support for installing a profile without requiring kubeconfig access #1371add profile
creates aHelmRelease
that references theHelmRepository
name and namespace.HelmRelease
exists with the same name/version/namespace/cluster combination in the config repository's Profiles manifest located at/.weave-gitops/clusters/<cluster name>/system/profiles.yaml
,add profile
appends the newHelmRelease
to it. Otherwise, it returns an error.profile.yaml
file for newHelmRelease
s and installs them.GitProvider
interface additionsThis PR implements
gitprovider.GetRepoFiles
to fetch a repo's subdirectory and its files. It's essentially a wrapper around go-git-providers to use.Files().Get(ctx, path, branch)
.add profile
uses this func to get the/weave-gitops/clusters/<cluster-name>/system/profiles.yaml
file from the config repo to read/appendHelmRelease
s to it.This PR also implements
gitprovider.MergePullRequest
to merge a PR through the Git provider API without relying on a local clone of the config repo.auto-merge
in the acceptance tests uses this function.How did you test it?
Manual Test
Acceptance Test
Release notes
Documentation Changes
Not sure where to add docs for this? 🤔