-
Notifications
You must be signed in to change notification settings - Fork 62
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
Allow for custom packages #100
Conversation
Signed-off-by: Manabu Mccloskey <[email protected]>
I think we should enable both. ideally what would be effective is that the CLI entries for the |
I would break out custom package to its own top level resource rather than being a field on local build. I'm less concerned with the CLI operation (we can change that later easier) but by having this as its own resource it should be much easier to add/delete packages while idpbuilder is running via CLI or programatically. |
I think we also need to add some docs for how to write these custom packages. It seems like we've got a good handle on the special keyword we're adding to argocd applications at this point. |
Yeah I thought about this too. It became a bit messy as I was working through it because of dependency on the local build resource, so I chose the faster way. I will make it a top level resource.
100%. Once we finalize this, we should document it, update the Readme, record demo, etc. |
Signed-off-by: Manabu Mccloskey <[email protected]>
Signed-off-by: Manabu Mccloskey <[email protected]>
Signed-off-by: Manabu Mccloskey <[email protected]>
@nimakaviani @greghaynes Review again when you get a chance please. Some notes: |
Signed-off-by: Manabu Mccloskey <[email protected]>
33272e8
to
1aa1601
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.
This looks great!
func (r *LocalbuildReconciler) reconcileCustomPkg(ctx context.Context, resource *v1alpha1.Localbuild, pkg v1alpha1.CustomPackageSpec) (ctrl.Result, error) { | ||
logger := log.FromContext(ctx) | ||
|
||
sc := runtime.NewScheme() |
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.
Any reason we're not using the existing scheme? https://github.com/cnoe-io/idpbuilder/pull/100/files#diff-4366f8cbb88710d711ad11de9ad64640db3d3ce472c270f315b204a3f6906609R51 ?
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.
sorry I left this a bit ago and forgot to include it in a review - looks like this got fixed though?
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.
yeah it's fixed :)
@nabuskey When I run this locally it exits before installing with:
Are we now making packageDirs a required parameter? What do we think the default command to recommend to users should be? |
Signed-off-by: Manabu Mccloskey <[email protected]>
That was my bad. I did not mean to make that field a required command flag. Fixed now. |
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.
some minor changes, otherwise looks good
// CustomPackageSpec controls the installation of the custom applications. | ||
type CustomPackageSpec struct { | ||
// Replicate specifies whether to replicate remote or local contents to the local gitea server. | ||
Replicate bool `json:"replicate"` |
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 can be defaulted to something, no? I assume it is probably a no
by default?
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.
Yeah it defaults to false via zero value. I'll add an annotation to be explicit.
InternalGitServeURL string `json:"internalGitServeURL"` | ||
GitServerAuthSecretRef SecretReference `json:"gitServerAuthSecretRef"` | ||
|
||
ArgoCD ArgoCDPackageSpec `json:"argoCD,omitempty"` |
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.
PackageSpec
maybe? this leaves room for us to hook in other packaging tools later on too, without having to changeg the spec
api/v1alpha1/custom_package_types.go
Outdated
} | ||
|
||
type CustomPackageStatus struct { | ||
Synced bool `json:"synced,omitempty"` |
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.
should it contain the git repo for where the spec is pushed and tracked? particularly if it is in gitea?
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.
Good point.
pkg/cmd/create/root.go
Outdated
@@ -60,10 +62,40 @@ func create(cmd *cobra.Command, args []string) error { | |||
os.Exit(1) | |||
} | |||
|
|||
b := build.NewBuild(buildName, kubeVersion, kubeConfigPath, kindConfigPath, extraPortsMapping, k8s.GetScheme(), ctxCancel) | |||
var absPaths []string |
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.
maybe a more descriptive name? absDirPaths
?
populate status field Signed-off-by: Manabu Mccloskey <[email protected]>
Next time, please update the README.md file too ! @nabuskey |
There is an open issue for it with folks already showing interest #103 Manabu chose to let the community help there, if possible. We will pick it up if no traction from the community occurs soon-ish |
This PR allows for end users to specify additional packages to be installed.
Example command:
The contents of
testDir2
:This contains a single manifest that defines an ArgoCD Application. This is passed directly to ArgoCD.
In
testDir
, it containsapp.yaml
. This file is an ArgoCD application except for one field,spec.source.repoURL
.The field above specifies the local path that contains manifests to be synced to Gitea repository. It will create a Gitea repository, fill it with contents from the busybox directory, then crates an ArgoCD application.
This will also continuously sync local file contents to the repository as long as the controller manager does not exit.
You can also add more ArgoCD application files to the specified directories, and the reconciler will pick them up.
Notes
I am not sure if the API is what we want. Initially I thought to do something like
This means Argo apps are defined in CLI and can't add more apps unless you exit the process and run the command again. Not a deal breaker imo but the other way is more user friendly? I don't personally have a preference so any suggestions here is welcome.