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

[DOC] Supporting doc changes for additional CRD status fields #1833

Merged
merged 11 commits into from
Jul 25, 2019
Merged

[DOC] Supporting doc changes for additional CRD status fields #1833

merged 11 commits into from
Jul 25, 2019

Conversation

d-laing
Copy link
Member

@d-laing d-laing commented Jul 23, 2019

Type of change

  • Documentation

Description

  • Updated the existing Strimzi custom resource status concept module to summarize every resource that now has status information. The kafkaTopic resource is included as a placeholder -- the development work is still outstanding.

  • Explained the new observedGeneration information that is available for every custom resource. Can we add any more information about this?

  • Removed the note that the status property is still under development and only available for kafka.

  • Minor changes to the Checking the status of a custom resource procedure.

Checklist

Please go through this checklist and make sure all applicable tasks have been done

  • Update/write design documentation in ./design
  • Write tests
  • Make sure all tests pass
  • Update documentation
  • Check RBAC rights for Kubernetes / OpenShift roles
  • Try your changes from Pod inside your Kubernetes and OpenShift cluster, not just locally
  • Reference relevant issue(s) and close them after merging
  • Update CHANGELOG.md

@@ -1058,13 +1058,13 @@ Used in: xref:type-KafkaBridgeStatus-{context}[`KafkaBridgeStatus`], xref:type-K
|Property |Description
|type 1.2+<.<|The unique identifier of a condition, used to distinguish between other conditions in the resource.
|string
|status 1.2+<.<|The status of the condition, one of True, False, Unknown.
|status 1.2+<.<|The status of the condition, either True, False or Unknown.
Copy link
Member

Choose a reason for hiding this comment

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

This is generated file. You have to do these changes in the strimzi-kafka-operator/api/src/main/java/io/strimzi/api/kafka/model/status/Condition.java and regenerate doc.

Copy link
Member

Choose a reason for hiding this comment

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

@laidan6000 Do you want to try to fix it your self? Or should I open a separate PR to fix this for you?

Copy link
Member Author

Choose a reason for hiding this comment

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

Please can you fix in a separate PR, @scholzj . I'll attempt to back out the change.

Copy link
Member

Choose a reason for hiding this comment

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

@laidan6000 #1836 - please review

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks good. To revert the changes to appendix_crds.adoc, can I just delete the file in GitHub? Click ... > Delete File.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure. I would expect that to delete the whole file and not just the changes. I'm not a Git expert, but I think you should be able to:

  • Fetch the data from upstream using git fetch
  • Checkout the single file from the master branch: git checkout origin/master documentation/book/appendix_crds.adoc

(perhaps make a backup of the local repository before trying this - make a copy before doing this :-o)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll try this... 😰 😄

Copy link
Member

Choose a reason for hiding this comment

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

Give it a try. Or if you want me or whoever will be merging this can fix it for you while merging it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Please can you fix it during the merge?

Copy link
Member

Choose a reason for hiding this comment

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

Sure, i will.

@scholzj scholzj added this to the 0.13.0 milestone Jul 24, 2019
Copy link
Member

@tombentley tombentley left a comment

Choose a reason for hiding this comment

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

One minor thing, otherwise LGTM

documentation/book/con-custom-resources-status.adoc Outdated Show resolved Hide resolved
Copy link
Member

@ppatierno ppatierno left a comment

Choose a reason for hiding this comment

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

LGTM. Just a couple of things.

documentation/book/con-custom-resources-status.adoc Outdated Show resolved Hide resolved

* `KafkaUserStatus` provides the user name of the Kafka user and the `Secret` in which their credentials are stored.

* `KafkaBridgeStatus` provides the HTTP address at which external client applications can access the Bridge service.
Copy link
Member

Choose a reason for hiding this comment

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

maybe we should say "HTTP based URL" based on the naming we are using in the implementation. Wdyt @tombentley ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Or:
"provides the URL at which external client applications can access the Bridge service"

Daniel Laing and others added 2 commits July 25, 2019 14:45
Co-Authored-By: Tom Bentley <tombentley@users.noreply.github.com>
@scholzj scholzj merged commit f7983ea into strimzi:master Jul 25, 2019
@d-laing d-laing deleted the additional-crd-status-doc branch July 25, 2019 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants