Skip to content
This repository has been archived by the owner on Aug 25, 2021. It is now read-only.

Add last synced time to CRDs #849

Merged
merged 1 commit into from
Mar 4, 2021
Merged

Conversation

thisisnotashwin
Copy link
Contributor

@thisisnotashwin thisisnotashwin commented Mar 4, 2021

Changes proposed in this PR:

  • Update CRDs to have printer column for "Last Synced At" which displays time since the last successful sync of a resource with the Consul Server.
    Companion PR to Add LastSyncedTime field on CRDs consul-k8s#448
    How I've tested this PR:
  • Deployed consul-helm with the image: ashwinvenkatesh/consul-k8s@sha256:8f2d59278f4b8deca8eaeb643a3e6143684e4c37a3e03c051781a1c42a8d70d4 for consul-k8s
  • Created a resource and kubectl get resource now displays a column Last Synced At with time since the successful sync.

image

How I expect reviewers to test this PR:

  • Retry the above step. Potentially edit the object to observe the Last Sync time update itself.
    Edit the config with a "failing" spec and save it. The Last Synced time should now not update as the sync will be unsuccessful.

Checklist:

  • Bats tests added
  • CHANGELOG entry added (HashiCorp engineers only, community PRs should not add a changelog entry)

Copy link
Contributor

@kschoche kschoche left a comment

Choose a reason for hiding this comment

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

Looks great!
Curious if the two descriptions should be the same text and also should both descs end with a period? If not it looks 💯

@thisisnotashwin thisisnotashwin added enhancement New feature or request theme/crds labels Mar 4, 2021
@thisisnotashwin thisisnotashwin marked this pull request as ready for review March 4, 2021 15:49
@thisisnotashwin
Copy link
Contributor Author

Curious if the two descriptions should be the same text and also should both descs end with a period?

@kschoche I looked at what we have done previously and it seems consistent with the way that we have setup other printer columns and also descriptions for fields.

Copy link
Member

@lkysow lkysow 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 verifying you use the make ctrl-crd-copy helm=<path-to-consul-helm-repo> command?

@thisisnotashwin
Copy link
Contributor Author

Just verifying you use the make ctrl-crd-copy helm= command?

@lkysow I did and it was glorious!

@thisisnotashwin thisisnotashwin merged commit 9afb6bb into master Mar 4, 2021
@thisisnotashwin thisisnotashwin deleted the add-last-synced-time branch March 4, 2021 20:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request theme/crds
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants