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

Improve setters API: SetterDefinition and SetterValues #985

Closed
3 of 4 tasks
frankfarzan opened this issue Aug 27, 2020 · 13 comments
Closed
3 of 4 tasks

Improve setters API: SetterDefinition and SetterValues #985

frankfarzan opened this issue Aug 27, 2020 · 13 comments
Assignees
Labels
p1 size/XXL 16 day triaged Issue has been triaged by adding an `area/` label workflow-blocker

Comments

@frankfarzan
Copy link
Contributor

frankfarzan commented Aug 27, 2020

Currently, kpt cfg set is imperative per-setter command. We need a way for user to declare all setter values in a configuration file. As part of this, distinguish setter definition (provided by package publisher) from setter values provided by the consumer.

  • Implement initial version of setter Definitions by setting default values
  • Implement setterValues
  • Implement setting multiple values
  • Implement setter validations
@frankfarzan frankfarzan added this to Backlog in Config as Data - Milestone 3 via automation Aug 28, 2020
@frankfarzan frankfarzan moved this from Backlog to Week/34 in Config as Data - Milestone 3 Aug 28, 2020
@phanimarupaka phanimarupaka added area/cfg p0 size/L 4 day triaged Issue has been triaged by adding an `area/` label labels Aug 29, 2020
@frankfarzan frankfarzan added p1 and removed p0 labels Sep 2, 2020
@phanimarupaka phanimarupaka moved this from Week/34 to Week/36 in Config as Data - Milestone 3 Sep 2, 2020
@phanimarupaka phanimarupaka moved this from Week/36 to Week/38 in Config as Data - Milestone 3 Sep 15, 2020
@phanimarupaka phanimarupaka added this to Backlog in Config as Data - Milestone 4 via automation Sep 30, 2020
@phanimarupaka phanimarupaka moved this from Backlog to Week/40 in Config as Data - Milestone 4 Sep 30, 2020
@phanimarupaka
Copy link
Contributor

phanimarupaka commented Oct 2, 2020

@pwittrock Here is the proposal for declarative setters UX. Please go through this and feel free to comment.

@phanimarupaka phanimarupaka moved this from Week/40 to Week/42 in Config as Data - Milestone 4 Oct 13, 2020
@mikebz mikebz added this to To do in kpt kanban board via automation Oct 23, 2020
@mikebz mikebz moved this from To do to In progress in kpt kanban board Oct 23, 2020
@phanimarupaka phanimarupaka moved this from In progress to To do in kpt kanban board Oct 27, 2020
@phanimarupaka phanimarupaka moved this from To do to In progress in kpt kanban board Oct 27, 2020
@phanimarupaka phanimarupaka moved this from In progress to To do in kpt kanban board Oct 28, 2020
@frankfarzan frankfarzan changed the title Declarative UX for providing setter values Improve setters API: SetterDefinition and SetterValues Nov 11, 2020
@yhrn
Copy link

yhrn commented Nov 19, 2020

Hi, has there been any progress in this area recently?

@phanimarupaka
Copy link
Contributor

We are actively working on Search and Replace functionality which is a prerequisite for this feature #961 . We plan to deliver this feature early next year. I will keep you posted with updates .

@phanimarupaka phanimarupaka moved this from To do to In progress in kpt kanban board Jan 19, 2021
@phanimarupaka phanimarupaka added size/XXL 16 day and removed size/L 4 day labels Jan 19, 2021
@phanimarupaka phanimarupaka moved this from In progress to In Review in kpt kanban board Jan 20, 2021
@phanimarupaka phanimarupaka moved this from In Review to To do in kpt kanban board Jan 28, 2021
@phanimarupaka phanimarupaka moved this from To do to In progress in kpt kanban board Feb 1, 2021
@phanimarupaka phanimarupaka moved this from In progress to To do in kpt kanban board Feb 16, 2021
@marshall007
Copy link

@phanimarupaka could you provide another status update here? It appears that the search/replace functionality in #961 has all been implemented but I am having trouble tracking down related MRs and doc. We are interested in this functionality as well as improved setters UX proposal you linked above.

I wanted to chime in on the setters UX proposal, but wasn't sure if that was still being actively discussed. There was mention of object setters in this comment. I wanted to add that we have a similar use case around "file" setters that keeps popping up in our implementations.

We often find ourselves having to manually document and inform users of files that are expected to be present at certain paths for Kustomize components to function properly. It would be great if we could instead document these requirements as if they were any other "required" setter. Something along the lines of:

kpt cfg set dir/ default-cert.tls.key --file /path/to/tls.key
kpt cfg set dir/ default-cert.tls.crt --file /path/to/tls.crt

We were thinking this something like this --file argument could work seamlessly for both existing string type setters as well as a new file type setter:

  • string: the contents of the file at the specified path are read and written to the setter value
    • in other words, the above command would be equivalent to kpt cfg set dir/ default-cert.tls.key "$(cat /path/to/tls.key)"
  • file: for file type setters, the value is expected to be a path with the kpt package. when --file is used on a file type setter, the contents are read and written to the path specified by the setter value
    • if a file already exists at that path, it is overwritten. if it does not already exist, it is created
    • if a string value is provided instead of the --file argument on a file type setter, that string value gets written to the path
  • *: if the user attempts to use the --file argument on anything other than string/file type setters, an error is thrown

WDYT?

@phanimarupaka
Copy link
Contributor

phanimarupaka commented May 6, 2021

Hi @marshall007, thanks for the comment. Here is the new version of set operation, a kpt function named apply-setters. We have also significantly simplified the experience of creating setters using our new search-replace function. Please try them out and let us know your feedback. You can provide the values to setters in a ConfigMap and apply them all at once, rather than setting them one at a time. We do not have plans to support reading from file in the near future but you may file a separate issue for record. I feel with this new declarative approach will suffice your needs.

@marshall007
Copy link

marshall007 commented May 7, 2021

@phanimarupaka great, thanks! We don't have much experience using kpt functions yet, but this looks fantastic! I will give this a shot before filing a separate issue. Looking at this, I do have a few more questions if you don't mind:

Am I understanding the implementation of apply-setters/applysetters/apply_setters.go correctly in that it is strictly looking for comment nodes with the hardcoded # kpt-set prefix and is thus incompatible with existing # {"$kpt-set":"..."} style definitions?

I also don't see any references to a Kptfile and/or Krmfile. Is this intended to be a complete departure from the package's OpenAPI schema definitions or just a temporary stop-gap? I can appreciate the simplicity of making the schema optional for trivial use-cases, but we would definitely prefer to be able to validate the function input against a well-defined schema (and continue using it for doc generation).

Finally, is there going to be any attempt at improving compatibility between tooling that is based on kyaml? We are finding it frustrating that kpt uses different naming conventions for setters and has no interop with Krmfiles. It would be really great if you could pull in a remote package with kpt and not have to worry about which tooling the author of said package decided to use when defining setters/functions. Likewise, we'd really like to be able to maintain and publish our packages with kpt and still allow consumers to use the kustomize CLI if they want.

@phanimarupaka
Copy link
Contributor

phanimarupaka commented May 7, 2021

@phanimarupaka great, thanks! We don't have much experience using kpt functions yet, but this looks fantastic! I will give this a shot before filing a separate issue. Looking at this, I do have a few more questions if you don't mind:

Am I understanding the implementation of apply-setters/applysetters/apply_setters.go correctly in that it is strictly looking for comment nodes with the hardcoded # kpt-set prefix and is thus incompatible with existing # {"$kpt-set":"..."} style definitions?

Yes. They are incompatible. But existing kpt packages can be easily migrated to new version in an automated fashion. The work is in progress and you can track the progress in this epic #1834. For customers like you using setters, migration can be done in a button click.

I also don't see any references to a Kptfile and/or Krmfile. Is this intended to be a complete departure from the package's OpenAPI schema definitions or just a temporary stop-gap? I can appreciate the simplicity of making the schema optional for trivial use-cases, but we would definitely prefer to be able to validate the function input against a well-defined schema (and continue using it for doc generation).

Yes. We want to make simple use-cases very simple. They cover most of the customer use-cases. For advanced use-cases of openapi constraints, we are figuring out the better UX to retain simplicity and provide an easy way for package publishers to provide the openapi schema. It could be persisted in either Kptfile or in some other meta file is something in discussions. We want to have an optimized UX for package consumers.

Finally, is there going to be any attempt at improving compatibility between tooling that is based on kyaml? We are finding it frustrating that kpt uses different naming conventions for setters and has no interop with Krmfiles. It would be really great if you could pull in a remote package with kpt and not have to worry about which tooling the author of said package decided to use when defining setters/functions. Likewise, we'd really like to be able to maintain and publish our packages with kpt and still allow consumers to use the kustomize CLI if they want.

Currently the experience of KRM functions is optimized for kpt as you mentioned. @mikebz will be the better person to take this question.

@mikebz
Copy link
Contributor

mikebz commented May 7, 2021

@marshall007 just a quick note on the long term thought process around functions. The long term vision is to make sure that functions can be used in a variety of products for customization or validation of configuration. We need to make them work really well in kpt first and then definitely look at how to make sure the same functions work well in kustomize scenarios as well. Right now kustomize function support is in Alpha so we are expecting a few rough edges.

@marshall007
Copy link

@mikebz thanks, that makes sense regarding the implementation of functions themselves. What I was more interested in is whether or not there was a plan to standardize around the conventions/naming of things. Would you mind commenting on this aspect as well? Examples include:

  • despite Kptfile being (afaik) a strict superset of the Krmfile spec, tooling to manipulate them is completely independent/incompatible
  • kustomize uses a very generic # {"$openapi":"..."} naming convention for setter comment nodes
  • kpt uses a very specific # {"$kpt-set":"..."} naming convention, making it incompatible
  • the new apply-setters kpt function now uses # kpt-set:, which is incompatible with both

It seems like it would make sense to take advantage of the opportunity that pivoting to functions presents and go back to a standard naming convention (like # openapi:, for example) so that, in the future, when kustomize function support is graduated out of alpha both tools can use the same conventions for setters.

Thanks in advance!

@mikebz
Copy link
Contributor

mikebz commented May 11, 2021

@droot @monopole can you please comment?

kpt kanban board automation moved this from To do to Done Jun 22, 2021
@frankfarzan
Copy link
Contributor Author

Closing this given the original scope of the issue. Please file separate issues if there are outstanding questions/suggestions.

@bgrant0607
Copy link
Contributor

I filed #2369 to propose making the "kpt-set" anchor string configurable.

@bgrant0607
Copy link
Contributor

I don't think Krmfile should exist.
kubernetes-sigs/kustomize#3953

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p1 size/XXL 16 day triaged Issue has been triaged by adding an `area/` label workflow-blocker
Projects
Development

No branches or pull requests

7 participants