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

Log the kind config file generated by our IDP builder #63

Merged
merged 3 commits into from
Nov 6, 2023

Conversation

cmoulliard
Copy link
Contributor

@cmoulliard
Copy link
Contributor Author

To be reviewed @greghaynes @nabuskey

@nabuskey
Copy link
Collaborator

Sorry I don't think I was not specific at all in the issue. What I was thinking is to introduce a new subcommand like idpbuilder print. When invoked, it prints embedded things like Kind config file, ArgoCD manifests, Gittea manifests.

I think printing kind config during build is useful for debugging purposes, but being able to do it without invoking build would be ideal.

@cmoulliard
Copy link
Contributor Author

Sorry I don't think I was not specific at all in the issue. What I was thinking is to introduce a new subcommand like idpbuilder print. When invoked, it prints embedded things like Kind config file, ArgoCD manifests, Gittea manifests.

I like this idea but what could be the interest to export ArgoCD or Gitea manifest EXCEPT if we propose a mechanism to export/import ? Without such export/import command a user could get the argocd resources using "kubectl get applications, etc" @nabuskey

@nabuskey
Copy link
Collaborator

You are correct that the mechanism doesn't exist, but I am proposing such mechanism for Gitea here:

In case the embedded configuration does not work for some reason, we should provide a flag, `--git-manifest-file`. When used, it specifies a single file that contains manifests necessary to configure a git server.
It is important to note that the file contains raw manifests that will be applied as-is, not helm values. Supporting manifest rendering mechanisms come with a cost of needing to include and maintain libraries and light logic to render these manifests. One of the goals of this proposal is to minimize dependencies.
It's also difficult to ensure rendered manifests are what users intended to use. Users may have different versions of helm from our version. This may cause slight differences in manifests due to a bug or a feature differences.
Users are expected to run the `helm template` command to generate manifests.

Main use case to me is to allow for direct manifest manipulation to make small changes without rendering mechanisms. So the workflow I am thinking is something like:

  1. idpbuilder print --git > /tmp/file.yaml
  2. Make changes to /tmp/file.yaml
  3. Build. idpbuilder create ----git-manifest-file /tmp/file.yaml

For making small changes, it makes a lot easier to work from a known working configuration.

This applies to the initialization phase (the create subcommand) only in the doc, but there's no reason we can't make it apply after the phase. E.g. hypothetical subcommand reconfigure under root.

@cmoulliard
Copy link
Contributor Author

When used, it specifies a single file that contains manifests necessary to configure a git server.

Why do you want to only export/import the git server manifest AND not all the core components (= git + argocd + ingress + etc) OR a specific core component to be able to re-apply it in case we have discovered an issue during the creation of the IDP cluster and would like to fix it by re-creating a new cluster using the file of the manifests ?

Remark: What you would like to do could break the IDP cluster if you change the service, ports, ingress host and dont change the king cfg, etc.

@cmoulliard
Copy link
Contributor Author

  1. idpbuilder print --git > /tmp/file.yaml

What do you think about the following commands ?
idpbuilder export component/git > /tmp/file.yaml where we could propose as option -o <format>. yaml is the default format and perhaps the only one to be supported. So we could by default avoid to pass -o yaml as we will assume that user is asking to export the manifest as YAML.

Remark: The manifest exported , should be "cleaned" to avoid to get uid, annotations generated, etc

and when we create an idp cluster =>

idpbuilder create --component git -f /tmp/file.yaml
idpbuilder create --component git,argocd -f /tmp/file.yaml
idpbuilder create --component git,argocd,ingress -f /tmp/file.yaml

@nabuskey
Copy link
Collaborator

Like you pointed out, the absolute minimum packages we need for idpbuilder to function are:

  1. Gittea.
  2. ArgoCD
  3. Ingress for ArgoCD and Gittea.

I agree that we should allow for ArgoCD and ingress configurations in a similar way. It just wasn't pointed out in the section since it talks about Git server. I'll add it to the doc.

Remark: What you would like to do could break the IDP cluster if you change the service, ports, ingress host and dont change the king cfg, etc.

Agreed. I think we can document that in the readme as well as in the manifest we print to warn users about these potential pitfalls. We can of course attempt to correct these in code but I am a bit hesitant without a concrete ask.

I like the export subcommand btw. Similar to Flux :)

where we could propose as option -o . yaml is the default format and perhaps the only one to be supported. So we could by default avoid to pass -o yaml as we will assume that user is asking to export the manifest as YAML.

I think we should export with yaml format only. Primary reason is that YAML allows for comments natively. Our comments in the document around potentially breaking fields, like ports you mentioned above, is very useful for end users to protect themselves. I am assume you meant to allow for JSON format here. If not, please clarify.

idpbuilder create --component git,argocd,ingress -f /tmp/file.yaml

I don't think this is explicit enough. If we are to bundle everything (manifests for Argo, Gittea, and Ingresses) in a single file, It's hard to figure out which manifest belongs to which package.
I think having three separate flags for the three absolute minimum packages is okay. HOWEVER, if we are to expand these in the future, we need something similar to what you wrote.

So to be more specific, we could do something like:

idpbuilder create --core-pakcage git -f /tmp/git.yaml --core-package argocd -f /tmp/argocd.yaml

Then use stringslice to receive flag values. Let me know what you think!

What we are talking about here is definitely out of scope for this issue though. Once we have a good way forward, let's open another issue to track this part.

@greghaynes
Copy link
Contributor

Have we considered adding this as a flag to the build command? I'd worry that if we create a second command it would also need to be invoked with the same parameters as the corresponding build command otherwise it may not be able to print out matching configuration. Alternatively, we could inject enough state in to k8s at build time that we could read it back out to print...

@nabuskey
Copy link
Collaborator

Have we considered adding this as a flag to the build command? I'd worry that if we create a second command it would also need to be invoked with the same parameters as the corresponding build command otherwise it may not be able to print out matching configuration. Alternatively, we could inject enough state in to k8s at build time that we could read it back out to print...

Not quite following. Can you elaborate a bit more?

Are you worried about duplicating flags / logic for export and build commands? I don't really have a preference on where the logic. lives. I think being able to export embedded default manifests (not runtime manifests), is a nice way to customize things quick.

@greghaynes
Copy link
Contributor

Imagine we have some of the flags for configuration mentioned here like --component a,b,c. I first run idpbuilder create --component a,b,c, if I then run idpbuilder export-config do I need to still specify --component a,b,c? If I dont, how would the export-config command know my configuration. Another trivial example would be if a user ran idpbuilder --kind-config kind.yaml, if we do idpbuilder export-config it needs to know to export kind.yaml not the embedded kind config.

@nabuskey
Copy link
Collaborator

Oh I see, you are talking about exporting run time configuration. I was thinking about exporting embedded default configuration that we know will work with the binary. So the output of export command would be static to the binary release.

My intention was to allow for users to edit the known working configuration, then create a cluster off of it. Since we are using kind clusters, re-creating a cluster is fast.

Now you mentioned it, wonder if we will confuse users. They might take it as exporting the current running configuration.

@greghaynes
Copy link
Contributor

yeah i think this feature probably makes a lot of sense in the context of a build log, personally

@greghaynes
Copy link
Contributor

one more thought about manipulating the manifests - if we are using gitea would you be able to do this for most things with git clone/edit/push?

@nabuskey
Copy link
Collaborator

one more thought about manipulating the manifests - if we are using gitea would you be able to do this for most things with git clone/edit/push?

Yeah once we bootstrap, we can hand off management of everything to ArgoCD.

@cmoulliard
Copy link
Contributor Author

3. Ingress for ArgoCD and Gittea.

Remark: ingress is needed for ALL the deployment (and not only for ArgoCD, Gitea) where a UI or URL will be exposed outside of the cluster (= to be able to access it from the local, laptop machine)

@cmoulliard
Copy link
Contributor Author

What we are talking about here is definitely out of scope for this issue though. Once we have a good way forward, let's open another issue to track this part.

+100

@cmoulliard
Copy link
Contributor Author

idpbuilder create --core-pakcage git -f /tmp/git.yaml --core-package argocd -f /tmp/argocd.yaml

What about such command ?

idpbuilder create --core git -f /tmp/git.yaml --core argocd -f /tmp/argocd.yaml

@cmoulliard
Copy link
Contributor Author

As this PR is relevant as it logs the kind config generated, can we merge it and create a separate issue concerning the discussion around export/import core packages YAML resources ? @nabuskey

Copy link
Collaborator

@nabuskey nabuskey left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! Left a couple of comments.

@cmoulliard We had a discussion about the issue during the technical meeting today. I have put a summary here: #58 (comment)

README.md Outdated Show resolved Hide resolved
@@ -90,6 +90,10 @@ func (c *Cluster) Reconcile(ctx context.Context, recreate bool) error {
return err
}

fmt.Print("########################### Our kind config ############################\n")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any objections to using a logger? I know it uses fmt here and other places, but I feel like it's nicer to consolidate logging stuff to one way. We currently use mix of fmt and logger and we should consolidate to one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with you. concerning this point, I commented this ticket #15 to propose a better way to log.
We will fix that in a separate PR

@cmoulliard
Copy link
Contributor Author

Could be merged now :-) @nabuskey

Copy link
Collaborator

@nabuskey nabuskey left a comment

Choose a reason for hiding this comment

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

LGTM

@nabuskey nabuskey merged commit cec784e into cnoe-io:main Nov 6, 2023
2 checks passed
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

Successfully merging this pull request may close these issues.

3 participants