-
Notifications
You must be signed in to change notification settings - Fork 64
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
Dynamic v3/info endpoint #3248
Dynamic v3/info endpoint #3248
Conversation
helm/korifi/values.yaml
Outdated
name: "" | ||
version: 0 | ||
minCLIVersion: "" | ||
minRecommendedCLIVersion: "" |
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.
Same as above, renaming minRecommendedCLIVersion
to recommendedCLIVersion
would probably make sense
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.
I've changed it.
helm/korifi/values.yaml
Outdated
@@ -44,6 +44,16 @@ api: | |||
idle: 900 | |||
readHeader: 10 | |||
|
|||
infoConfig: | |||
build: "" |
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.
The API docs are not really clear on what build
semantic is. Is there a use-case where this value is used? If not, we would prefer a lean approach and only put stuff that are actually needed.
Other than that, the docs example value sounds like a commit SHA. If that is the case, the same approach as to the version
should be applied (i.e. should not be configurable via a helm value)
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.
I think the v3/info endpoint in general is very open for interpretation and how you as an operator use and configure it. I think the 'old' approach of cf-deployment to put here the current release number is:
cloudfoundry/cf-deployment@ab93783
For my future usage, I would probably put here not a Korifi specific hash tag alone, but my own interpretation of the current build (some semver for my current Korifi + Kubernetes + cert-manager + ... setup).
Hey @gogolok A general question: do you know use cases where each property of the |
description: "" | ||
name: "" | ||
version: 0 | ||
minCLIVersion: "" |
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.
I wonder whether it makes sense to default cli versions to something? No idea what default values should be 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.
On a second glance, it seems that we already have some "min version" code in place. I guess returning this constant in the /v3/info
presenter would make sense, moreover it is already used to error if cli version is tool low.
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.
For cf-deployment, those properties seem to be empty.
Some real-life output:
{"build":"v1.56.0","cli_version":{"minimum":"","recommended":""},"custom":{},"description":"","name":"cf-deployment","version":39,"links":{"self":{"href":"https://api.system.EXAMPLE.com/v3/info"},"support":{"href":"support@EXAMPLE.com"}}}
I think I would keep minimum and recommended version empty by default and let the operator decide how to use it.
For example if you would fix cf whoami
( #1724 ), you would probably require a new CF CLI version and the operator could point out the fact you need a specific CLI version.
I consider
https://github.com/cloudfoundry/korifi/blame/c353c9ecd6f55a14799d22bedfaf5a822d15d8cc/api/middleware/cli_version.go#L14
to be a hard requirement the Korifi release imposes, but the operator might be more strict.
As I said before, I tried to mimic the old behaviour. The only property I see critical is Besides from this, I only see for my scenario the following properties to be useful:
|
helm/korifi/values.yaml
Outdated
@@ -44,6 +44,14 @@ api: | |||
idle: 900 | |||
readHeader: 10 | |||
|
|||
infoConfig: | |||
description: "" | |||
name: "" |
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 we should default name
and description
so that it looks more similar to current CF systems. Something generic like:
name: "korifi"
description: "Korifi Cloud Foundry Environment"
Looks good @gogolok. Dropped a small comment inline. Just one more request - can you squash your commits into one and force push before we approve and merge this PR. This way we will have cleaner history. Thank you for your contribution! |
Is there a related GitHub Issue?
#2914
What is this change about?
Extend Korifi configuration to provide dynamic content to the endpoint
v3/info
.The previous step was to introduce static content:
#3239
Does this PR introduce a breaking change?
No
Acceptance Steps
Switch to this PR's branch, set the
infoConfig
configuration inhelm/korifi/values.yaml
to some nonempty values, run
./scripts/deploy-on-kind.sh korifi
and checkcf curl v3/info
after loggin into Korifi viacf
CLI.Tag your pair, your PM, and/or team
None
Further Information