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

docs: add parameterized CMPs proposal #8103

Merged
merged 60 commits into from
Mar 16, 2022

Conversation

crenshaw-dev
Copy link
Member

@crenshaw-dev crenshaw-dev commented Jan 5, 2022

No description provided.

Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
@codecov
Copy link

codecov bot commented Jan 5, 2022

Codecov Report

Merging #8103 (452de2b) into master (722fe92) will increase coverage by 0.43%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8103      +/-   ##
==========================================
+ Coverage   41.53%   41.97%   +0.43%     
==========================================
  Files         174      178       +4     
  Lines       22703    22894     +191     
==========================================
+ Hits         9430     9610     +180     
+ Misses      11932    11905      -27     
- Partials     1341     1379      +38     
Impacted Files Coverage Δ
controller/sync.go 56.96% <0.00%> (-8.17%) ⬇️
server/cache/cache.go 68.00% <0.00%> (-5.34%) ⬇️
server/application/application.go 31.20% <0.00%> (-1.57%) ⬇️
util/session/sessionmanager.go 68.75% <0.00%> (-1.37%) ⬇️
controller/appcontroller.go 51.82% <0.00%> (-0.36%) ⬇️
util/localconfig/localconfig.go 2.77% <0.00%> (-0.32%) ⬇️
server/project/project.go 52.50% <0.00%> (-0.19%) ⬇️
util/helm/cmd.go 28.48% <0.00%> (-0.17%) ⬇️
util/clusterauth/clusterauth.go 66.66% <0.00%> (-0.16%) ⬇️
cmd/argocd/commands/headless/headless.go 3.48% <0.00%> (-0.13%) ⬇️
... and 29 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 722fe92...452de2b. Read the comment docs.

#### Terms

* **Parameter announcement**: an instance of a data structure which describes an individual parameter that may be applied
to a specific Application.
Copy link
Contributor

@keithchong keithchong Jan 7, 2022

Choose a reason for hiding this comment

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

The current first class support for Helm in the UI has custom/specific interfaces, and they are different than, say, those for Kustomize. What were your thoughts on providing a general structure that will fit for all tools? Also, should we provide UI customizations that can be configured and contributed by each CMP, so they for example, can customize

  1. the ordering of their parameters (currently they are in alphabetical order)
  2. custom groups with titles. eg. instead of a.b.param1, a.b.param2 on different lines, why not show it as param1 and param2, under some logical grouping? These titles can be more descriptive and user-friendly and looking beyond, can be translated.
  3. Allow different font styles for different parameters. Eg. what if some parameters are required, so do you want the label to be bolded or underlined?
  4. etc.

Basically, should we give the CMP plugin owner the ability to customize the look and feel of the UI, based on some common UI configuration spec that we define?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good questions. I'll answer here and also by adding info to the proposal. Some of the questions are answered in the notes I haven't transferred into the proposal yet. 😆

should we give the CMP owner the ability to customize the look and feel of the UI...

Yep! I'll add details to the "Easy to use" goal. But the basic idea is that parameter announcements should be able to communicate to the UI how they should be displayed.

...based on some common UI configuration spec that we define

We should define some kind of schema. That schema will be on a spectrum from loosely- to strictly-defined.

On one end of the spectrum, we define a data structure that allows the user to describe the UI in excruciating detail. On the other end, we accept minimal information and let the UI make decisions from there.

Here's my suggestion: a parameter announcement should accept two strings, type and description. type tells the UI how to interpret description. If type is "string", the UI presents a simple test input box, and there's not really any need for further description. For other types, there will be a documented schema for the description field. For example, if the type is enum, description might contain a JSON list of acceptable values.

The UI would implement components for common parameter types (like string, enum, multiline text, and image). CMP developers would have to contribute UI code for new types. We could offer a way for CMPs to inject UI code for their custom types, but I'd consider that out of scope for this proposal.

the ordering of their parameters

The UI could respect the order of the list of parameter announcements from the CMP.

custom groups with titles

Each parameter announcement could include a section field. The values could be used for grouping and for group headers.

Allow different font styles for different parameters

That info could be included in the description field. To accommodate this, we'd probably want to define an initial description schema for the text type of "empty string or JSON object". That would be flexible enough to add features like formatting.

what if some parameters are required

I'm not aware of any required parameters for the existing config management tools. But if we decided this was important, we could add a required field to the parameter announcement spec.

Copy link
Member Author

Choose a reason for hiding this comment

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

@keithchong updated with some examples that should clarify.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should define some kind of schema. That schema will be on a spectrum from loosely- to strictly-defined.

Were you thinking this should be part of the plugin.yaml or a new file that the CMP needs to provide? I was trying to come up with the pros and cons. Should UI-specific details be separated out?

I do like the idea of having some core, base types, like what you described. I know there is an issue with arrays not being shown properly in the UI but that could be improved upon. (eg. show +/- to add or remove elements)

That info could be included in the description field.

I edited my comment after I posted it. We should also keep in mind to support translation for these user-friendly strings.

I'm not aware of any required parameters for the existing config management tools.

I not sure, but that could be something to consider in the general case. In the UI, if the field is left blank, it implies it is optional. How will the user know which fields are required and need to be filled in?

Copy link
Member Author

@crenshaw-dev crenshaw-dev Jan 7, 2022

Choose a reason for hiding this comment

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

Were you thinking this should be part of the plugin.yaml or a new file that the CMP needs to provide?

The schema of the parameter announcement would be documented, and then the CMP would adhere to that schema when producing a parameter announcement for a given application. My parameter announcement schema proposal: https://github.com/argoproj/argo-cd/blob/3c972d4211b8d48d379d9b0fbede5329a5c3cb0f/docs/proposals/cmp-parameters.md#parameter-announcement-list-schema

Should UI-specific details be separated out?
...
We should also keep in mind to support translation for these user-friendly strings.

I should rename the description field to metadata or something like that (presentation? interface?). I'm thinking of that field, not as a textual description of the parameter, but as a JSON object holding information about how the UI should present the parameter field.

How will the user know which fields are required and need to be filled in?

It's a good question, but I think that's the status quo for native config management tools. One way this could be implemented is by adding UI support for this announcement:

[
  {
    "name": "some-required-parameter",
    "metadata": "{\"required\": true}"
  }
]

Copy link
Contributor

@keithchong keithchong Jan 13, 2022

Choose a reason for hiding this comment

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

Hi Michael, not sure if you saw in the chat during today's contributors call, but Jesse mentioned using Open API to define the types.

One thing I was thinking about was the user friendliness/usability of the CMP UI. One thing I noticed was the current lack of (at least, from what I saw) validation of parameter values. Is the onus on the UI to perform validation on a specific type? Take for example, the Helm parameter replicaCount? If the user inadvertently enters a letter instead of a number, do we want up-front validation, or, we let the user proceed and let generation happen and then see the resulting failure (like it is now)? How about semantic validation? For semantic validation, it should be done by the CMP. Should there be validate (alongside generate and discover) and maybe validate should be done before generate. Thoughts?

Actually, there is a "Skip Schema Validation" option, so there will be errors when generating.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think validation is definitely something we should be ready to support with parameterized CMPs. I'm not sure what role Open API (or maybe JSON schema, if that's different?) might play, but certainly worth looking at.

I'm not sure we should define a validate field yet, since that would be adding a feature that doesn't exist for config management tools. But I could imagine validation info being part of the uiConfig field (renamed from metadata field described above). Putting that info in the uiConfig field for now would leave room for a short-term improvement until we're ready to fully-define a validation system.

Can you give an example of what you mean by semantic validation?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the Open API definition can be used to verify the validity of uiConfig, when the plugin author is customizing the look of the UI, at “development time”, in other words. It should be versioned.

For semantic validation, perhaps take the Helm example, where the user has defined a values.yaml file in the repo, and the parameter value for the path is correct. It’s fine syntactically. However, semantically, it’s wrong because the file is not in the default path that Helm expects. The UI can’t validate this semantically and it’s very specific to the plugin tool itself.

Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
@jopit
Copy link
Contributor

jopit commented Jan 7, 2022

Replicating the behaviour of the Helm UI's VALUE FILES field will be challenging, since populating the drop-down requires examining the contents of the git repo specified in the UI's application creation panel. I don't see how the plugin itself could do that. Maybe we'd need something special in the description field to specify building the list by matching the files in the repo against a pattern.

image

Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
@crenshaw-dev
Copy link
Member Author

@jopit does the plugin not have access to the contents of the repo? Seems like it definitely has to have that access in order to build manifests.

Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
@jopit
Copy link
Contributor

jopit commented Jan 7, 2022

@crenshaw-dev The plugin will have access at generation time (the repo is cloned into a shared volume), but I don't think it has access at the time the application is being created, which is when the prompt is needed.

@crenshaw-dev
Copy link
Member Author

@jopit gotcha. Is there any reason it couldn't have access to the repo at application creation time? Looks to me as if the native Helm config management code must be cloning the repo at application creation time to do things like populate the directory list and the parameters interface. Is there any reason the plugin couldn't take advantage of that just-cloned repo?

@jopit
Copy link
Contributor

jopit commented Jan 7, 2022

For the build-in helm, the parameters are different depending on the application, looking at the helm apps in https://github.com/argoproj/argocd-example-apps, the helm-dependency app has these parameters

image

but the helm-guestbook has these

image

@crenshaw-dev
Copy link
Member Author

Parameter announcements could be requested/produced on a per-application basis though, right? That's the assumption I've been working with.

Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
@jopit
Copy link
Contributor

jopit commented Jan 7, 2022

@crenshaw-dev I'm not actually sure how the built-in helm code gets those values. I had assumed it wouldn't be cloning the repo on-the-fly when the user is filling in the app creation panel (before the app is created), because that seems potentially expensive and slow. Could be wrong, though. @alexmt could you shed some light on how this works?

Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
@jopit
Copy link
Contributor

jopit commented Jan 7, 2022

Yeah, parameter announcements could/would be on per-application basis. Although the UI needs the announcements before the app is actually created. Which means the plugin is definitely going to need to be able to access the repo, and will have also have to be told the path to the manifests within that repo.

…image param

Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
```go
type ParameterDefinition struct {
// Name is the name of a parameter. (required)
Name string `json:"name"`
Copy link
Contributor

@jopit jopit Jan 11, 2022

Choose a reason for hiding this comment

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

Names should be unique, but are they unique only within the section, or should they be globally unique?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think they should be unique per-section. If my CMP has a parameter called, say, address that's applicable to all applications, I wouldn't want it to conflict if used in conjunction with a Helm chart that has a parameter of the same name.

Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
@crenshaw-dev crenshaw-dev marked this pull request as ready for review January 13, 2022 00:08
@crenshaw-dev
Copy link
Member Author

Marking this as ready for review since I think the bulk of the content is there. I'll add clarifications in response to comments. I'll probably also add screenshots of current parameters interfaces as well as mocked-up screenshots of the proposed parameters interfaces (hopefully with minimal differences).

Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
…vements-v2

docs: add examples for new schema proposal
Copy link
Collaborator

@alexmt alexmt left a comment

Choose a reason for hiding this comment

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

LGTM.

Disclaimer: me, @crenshaw-dev , and @leoluz interact every day, so we've discussed the proposed design offline and agreed on the design. So approving without asking additional questions :)

Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
@alexmt alexmt merged commit 89c05ec into argoproj:master Mar 16, 2022
wojtekidd pushed a commit to wojtekidd/argo-cd that referenced this pull request Apr 25, 2022
docs: add parameterized CMPs proposal (argoproj#8103)

Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>

Co-authored-by: Leonardo Luz Almeida <leonardo_almeida@intuit.com>
Signed-off-by: wojtekidd <wojtek.cichon@protonmail.com>
@crenshaw-dev crenshaw-dev deleted the cmp-parameters-proposal branch April 2, 2023 15:43
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.

6 participants