Skip to content

Generate helm docs for release consul-k8s 1.1.2#17568

Merged
curtbushko merged 6 commits intomainfrom
consul-k8s-release-1.1.2-helm-docs
Jun 5, 2023
Merged

Generate helm docs for release consul-k8s 1.1.2#17568
curtbushko merged 6 commits intomainfrom
consul-k8s-release-1.1.2-helm-docs

Conversation

@curtbushko
Copy link
Contributor

Description

Generate docs for consul-k8s 1.1.2 release.

  • In the case of bugs, describe how to replicate
  • If any manual tests were done, document the steps and the conditions to replicate
  • Call out any important/ relevant unit tests, e2e tests or integration tests you have added or are adding

-->

Links

PR Checklist

  • updated test coverage
  • external facing docs updated
  • appropriate backport labels added
  • not a security concern

@curtbushko curtbushko added type/docs Documentation needs to be created/updated/clarified pr/no-changelog PR does not need a corresponding .changelog entry pr/no-metrics-test backport/1.15 This release series is no longer active on CE. Use backport/ent/1.15. labels Jun 5, 2023
@curtbushko curtbushko requested a review from david-yu June 5, 2023 17:42
@curtbushko curtbushko requested a review from a team as a code owner June 5, 2023 17:42
@curtbushko curtbushko self-assigned this Jun 5, 2023
- [tests ((#h-tests))](#tests-h-tests)
- [Helm Chart Examples](#helm-chart-examples)
- [Customizing the Helm Chart](#customizing-the-helm-chart)
- [`global`](#h-global)
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like it's missing the top level hierarchy here? Not sure why that happened

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure... I just ran the generation...

Copy link
Contributor

Choose a reason for hiding this comment

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

I see what happened @eddie-rowe looks like you introduce some more text into the generated page via this PR: https://github.com/hashicorp/consul/pull/17032/files.

I assume safe to override your changes? We need to change the way we generate this file if you want those changes to persist as this page is actually maintained via golang code MDX generation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting - I don't remember changing any of those higher level values.. maybe that was from a merge with another branch or resolving conflicts?

My changes for this particular page were to fix these broken links related to cluster peering:

https://github.com/hashicorp/consul/pull/17032/files#diff-d8efaedf4cf96374cd592339830873b0c609b0e0e5ddf30e74810058ab5142abR307-R1919

Copy link
Contributor

Choose a reason for hiding this comment

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

@trujillo-adam @boruszak - do you have any perspective on these mentioned changes?

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to implement Eddie's changes in the source code we generate this reference docs from, but it looks like we are already redirecting properly -- I think it's safe to overwrite his changes.

Copy link
Contributor

@david-yu david-yu left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Copy link
Contributor

@im2nguyen im2nguyen left a comment

Choose a reason for hiding this comment

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

are these changes generated from consul-k8s? if not, we should make the updates in https://github.com/hashicorp/consul-k8s, we might only need this to update the header table of contents


- `domain` ((#v-global-domain)) (`string: consul`) - The domain Consul will answer DNS queries for
(Refer to [`-domain`](/consul/docs/agent/config/cli-flags#_domain)) and the domain services synced from
(Refer to [`-domain`](https://developer.hashicorp.com/consul/docs/agent/config/cli-flags#_domain)) and the domain services synced from
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason why you want to use absolute links instead? we prefer relative links whenever possible (same for the rest of these changes)

Copy link
Contributor

Choose a reason for hiding this comment

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

this is auto-generated from the helm chart! feel free to disregard this comment

it makes sense to have absolute links in the helm chart since folks could use that as a source of truth. we might need to update the auto-gen to remove "https://developer.hashicorp.com"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@im2nguyen Can you approve as this is blocking the final step of the release. If we want to change the helm chart then that needs to happen in the consul-k8s repo on several branches.

Copy link
Contributor

Choose a reason for hiding this comment

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

i'll pre-approve for now so you're not blocked, but can you revert "Consul Connect" -> "service mesh" it's a change that we would like not be reverted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've reverted the changes for Consul Connect and other places that shouldn't have been changed.

- `secretKey` ((#v-global-secretsbackend-vault-ca-secretkey)) (`string: ""`) - The key within the Kubernetes or Vault secret that holds the Vault CA certificate.

- `connectCA` ((#v-global-secretsbackend-vault-connectca)) - Configuration for the Vault service mesh CA provider.
- `connectCA` ((#v-global-secretsbackend-vault-connectca)) - Configuration for the Vault Connect CA provider.
Copy link
Contributor

Choose a reason for hiding this comment

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

@jkirschner-hashicorp and I updated "Consul Connect" -> "service mesh" which a more appropriate name for this. is there a reason why we're reverting it back?

im2nguyen

This comment was marked as duplicate.

im2nguyen

This comment was marked as duplicate.

Copy link
Contributor

@im2nguyen im2nguyen left a comment

Choose a reason for hiding this comment

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

pre-approving as to not block merging, however, we should revert any changes back to Consul Connect

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport/1.15 This release series is no longer active on CE. Use backport/ent/1.15. pr/no-changelog PR does not need a corresponding .changelog entry pr/no-metrics-test type/docs Documentation needs to be created/updated/clarified

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants