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

add docs for dapr annotate command #2576

Merged
merged 5 commits into from
Jul 6, 2022

Conversation

mukundansundar
Copy link
Contributor

@mukundansundar mukundansundar commented Jun 28, 2022

Signed-off-by: Mukundan Sundararajan 65565396+mukundansundar@users.noreply.github.com

Thank you for helping make the Dapr documentation better!

Please follow this checklist before submitting:

  • Commits are signed with Developer Certificate of Origin (DCO - learn more)
  • Read the contribution guide
  • Commands include options for Linux, MacOS, and Windows within codetabs
  • New file and folder names are globally unique
  • Page references use shortcodes instead of markdown or URL links
  • Images use HTML style and have alternative text
  • Places where multiple code/command options are given have codetabs

In addition, please fill out the following to help reviewers understand this pull request:

Description

This PR is the docs PR for CLI command dapr annotate.

Corresponding closed issue in CLI dapr/cli#870.
Corresponding PR: dapr/cli#873

Issue reference

Signed-off-by: Mukundan Sundararajan <65565396+mukundansundar@users.noreply.github.com>
@mukundansundar mukundansundar added this to the 1.8 milestone Jun 28, 2022
@mukundansundar
Copy link
Contributor Author

@jjcollinge I am seeing that the defaults are set to -1, but based on the code generally the defaults are ignored if it is -1 for the int fields. So in that fashion, should we remove the default values from the default column for those int fields in the annotate command as this leads to some discrepancy between the annotation default values posted here?
Example: dapr.io/sidecar-liveness-probe-delay-seconds defaults to 3 based on the above linked page, but the value is said to default to -1 in CLI which is actually discarded and not applied to the configuration.

@msfussell Wanted your opinion on the slight difference in the annotation names, there are flags like --liveness-probe-delay which corresponds to dapr.io/sidecar-liveness-probe-delay-seconds. For convenience sake, the sidecar and seconds prefix and suffix are omitted as otherwise the flag will be extremely long. Similarly there are other sidecar based annotations that have the sidecar prefix omitted. IMO that is ok. What is your opinion? Should the docs be modified in anyway to tie the flag to the corresponding annotation to make it more clear?

@yaron2 @jjcollinge @msfussell Have put this command as a preview feature command for now is that ok?

@mukundansundar mukundansundar marked this pull request as ready for review June 28, 2022 05:54
@mukundansundar mukundansundar requested review from a team as code owners June 28, 2022 05:54
@msfussell
Copy link
Member

@jjcollinge I am seeing that the defaults are set to -1, but based on the code generally the defaults are ignored if it is -1 for the int fields. So in that fashion, should we remove the default values from the default column for those int fields in the annotate command as this leads to some discrepancy between the annotation default values posted here? Example: dapr.io/sidecar-liveness-probe-delay-seconds defaults to 3 based on the above linked page, but the value is said to default to -1 in CLI which is actually discarded and not applied to the configuration.

@msfussell Wanted your opinion on the slight difference in the annotation names, there are flags like --liveness-probe-delay which corresponds to dapr.io/sidecar-liveness-probe-delay-seconds. For convenience sake, the sidecar and seconds prefix and suffix are omitted as otherwise the flag will be extremely long. Similarly there are other sidecar based annotations that have the sidecar prefix omitted. IMO that is ok. What is your opinion? Should the docs be modified in anyway to tie the flag to the corresponding annotation to make it more clear?

@yaron2 @jjcollinge @msfussell Have put this command as a preview feature command for now is that ok?

@mukundansundar - I would make the command and the annotations the same, then this is really specific. It is not as if we expect people to use this frequently. Also the odd this command is that this is now ONLY K8s specific and we really should avoid this, by having the -k option. This now is the only K8s command in the CLI. then annotations are a K8s thing, however I would still prefer to "force" the -k option. WDYT? Finally if this is preview feature then you need to add this here, even for the CLI https://docs.dapr.io/operations/support/support-preview-features/

@jjcollinge
Copy link
Contributor

@mukundansundar thanks for raising that point. It definitely makes sense to try and keep these consistent - although I am not sure Cobra actually allows us to remove the defaults from the flags (or set a nil default) which makes this a bit tricky. If you know of a way then please let me know. If not then we have to provide a int default value - we could set it to the same defaults as the runtime if it has a default but this would mean that we would always have to set those annotations (or check if the value is the default value and then ignored it). Wdyt?
I'd be happy to add the -k flag and fail if not provided as @msfussell suggested.

@jjcollinge
Copy link
Contributor

I've opened a PR that adds the -k flag to the annotate command - it only supports Kubernetes so will fail if the -k flag is not present.

Signed-off-by: Mukundan Sundararajan <65565396+mukundansundar@users.noreply.github.com>
@mukundansundar
Copy link
Contributor Author

@msfussell Changed docs please review ...


### Description

Add dapr annotations to a Kubernetes configuration.
Copy link
Member

Choose a reason for hiding this comment

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

I presume arguments here reflects this list https://v1-8.docs.dapr.io/reference/arguments-annotations-overview/
If so has this list been checked since I could not find dapr.io/sidecar-image as a value.

Copy link
Member

Choose a reason for hiding this comment

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

If the above is true (the list) then I suggest the following update.

Suggested change
Add dapr annotations to a Kubernetes configuration.
Add Dapr annotations to a Kubernetes configuration. This enables you to change the Dapr annotations on a deployment other than those specified in the deployment file. See [Kubernetes annotations]({{< ref arguments-annotations-overview >}}) for a full description of each annotation.

Copy link
Contributor Author

@mukundansundar mukundansundar Jul 6, 2022

Choose a reason for hiding this comment

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

@msfussell I believe that this command as it exists is not for all the annotations as it exists. I am also thinking that all annotations should not be supported via this command. I am planning on creating a proposal to change this command to follow a helm based --set based format for 1.9. I will make note of the annotations needed to allowed to changed there.

| `--readiness-probe-period` | | `-1` | The period to use for the readiness probe in the sidecar. Read more [here](https://kubernetes.io/docs/tasks/configure-pod-container/configure-liveness-readiness-startup-probes/#configure-probes). |
| `--readiness-probe-threshold` | | `-1` | The threshold to use for the readiness probe in the sidecar. Read more [here](https://kubernetes.io/docs/tasks/configure-pod-container/configure-liveness-readiness-startup-probes/#configure-probes). |
| `--readiness-probe-timeout` | | `-1` | The timeout to use for the readiness probe in the sidecar. Read more [here](https://kubernetes.io/docs/tasks/configure-pod-container/configure-liveness-readiness-startup-probes/#configure-probes). |
| `--resource, -r` | | | The resource to target to annotate |
Copy link
Member

Choose a reason for hiding this comment

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

What is --resource? When is this used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See examples with the -r flag ... It is the Kubernetes deployment resource that is the target for the annotate command ...

# Annotate multiple deployments by name in a chain
kubectl get deploy -o yaml | dapr annotate -k -r nodeapp - | dapr annotate -k -r pythonapp - | kubectl apply -f -

More examples in the file ...

@msfussell
Copy link
Member

@mukundansundar - Reviewed the docs.

msfussell and others added 3 commits July 5, 2022 17:31
Signed-off-by: Mukundan Sundararajan <65565396+mukundansundar@users.noreply.github.com>
@@ -15,7 +15,7 @@ For CLI there is no explicit opt-in, just the version that this was first made a
## Current preview features
| Feature | Description | Setting | Documentation | Version introduced |
| ---------- |-------------|---------|---------------|-----------------|
| **--image-registry** flag with Dapr CLI| In self hosted mode you can set this flag to specify any private registry to pull the container images required to install Dapr| N/A | [init CLI command reference]({{<ref "dapr-init.md#self-hosted-environment" >}}) | v1.7 |
| **--image-registry** flag in Dapr CLI| In self hosted mode you can set this flag to specify any private registry to pull the container images required to install Dapr| N/A | [CLI init command reference]({{<ref "dapr-init.md#self-hosted-environment" >}}) | v1.7 |
Copy link
Contributor Author

@mukundansundar mukundansundar Jul 6, 2022

Choose a reason for hiding this comment

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

leaving this as such for now only with a slight change in phrasing ... will have a discussion with Yaron after 1.8 release on whether to have Preview features or Preview commands in CLI or not ...
cc @msfussell @yaron2

Copy link
Member

@msfussell msfussell left a comment

Choose a reason for hiding this comment

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

LGTM

@msfussell msfussell merged commit 57676c2 into dapr:v1.8 Jul 6, 2022
@mukundansundar mukundansundar deleted the dapr-annotate-docs branch July 6, 2022 18:28
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.

4 participants