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

Document the module config required fields #261

Merged
merged 1 commit into from
Nov 30, 2023
Merged

Conversation

stefanprodan
Copy link
Owner

Changes:

  • Explain which #Config fields are used by Timoni to inject the name, namespace, module version and cluster version.
  • Explain which changes are acceptable for Timoni's entry point.
  • Move the Kubernetes min version constraint to #Config.

Ref: #251

@stefanprodan stefanprodan added the area/docs Improvements or additions to documentation label Nov 30, 2023
Copy link
Contributor

@jmgilman jmgilman left a comment

Choose a reason for hiding this comment

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

Just two minor nits, otherwise LGTM.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just a note on this:

if instance.config.test.enabled {
	apply: test: [ for obj in instance.tests {obj}]
}

Even though this is a conditional, the module author doesn't really get a chance to "optionally" write tests. The reason being:

  1. The conditional references config.test which makes it a required field
  2. If you leave config.test and just don't implement tests, the consumer of the package is left wondering what this test option does (and would probably be disappointed it does nothing).

So the options are you either implement tests or unwire all of the logic around tests (which IIRC was in a few more places). This is a very minor nitpick, so it's probably fine as is, but this was just my experience coming into it (perhaps we'll adopt the testing with jobs strategy, but it's not something we've historically practiced in this way).

Copy link
Owner Author

Choose a reason for hiding this comment

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

Oh yeah that's the issue with the current blueprint, which turned out it's not quite minimal. Will work on that next, planning to have more flavours for the init command and also allow custom blueprints.

@stefanprodan stefanprodan merged commit 7dae986 into main Nov 30, 2023
@stefanprodan stefanprodan deleted the mod-config-docs branch November 30, 2023 23:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/docs Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants