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.

Sign up for GitHub

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

Jump to bottom

Add creation timestamp to receipts on install #695

Merged
merged 6 commits into from
Mar 9, 2021
Merged

Add creation timestamp to receipts on install #695

merged 6 commits into from
Mar 9, 2021

Conversation

chriskim06
Copy link
Member

This PR adds the current time as the timestamp when a plugin is installed and leaves it alone when something is upgraded. I wasn't sure if we should do something about existing receipts with null creationTimestamp's but figured it would be ok without something.

/assign @ahmetb
/assign @corneliusweig

Fixes #632

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 27, 2021
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 27, 2021
corneliusweig
corneliusweig reviewed Mar 7, 2021
View reviewed changes
Copy link
Contributor

@corneliusweig corneliusweig left a comment

Choose a reason for hiding this comment

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

Thanks for taking a look. I have some small suggestions, but looks good overall!

integration_test/testutil_test.go Outdated Show resolved Hide resolved
integration_test/install_test.go Outdated Show resolved Hide resolved
integration_test/upgrade_test.go Show resolved Hide resolved
integration_test/upgrade_test.go Outdated Show resolved Hide resolved
@ahmetb
Copy link
Member

ahmetb commented Mar 9, 2021

adds the current time as the timestamp when a plugin is installed and leaves it alone when something is upgraded

Is there a particular reason why we leave it untouched during the upgrade? I’m ok with it, just curious why? To let user figure out when they first installed a plugin?

@chriskim06
Copy link
Member Author

Ya that was my thinking. It makes more sense to me that the creationTimestamp reflects when it was first installed. Even though upgrade is just reinstalling the newer version, I think it might be confusing to some users if that timestamp changed after an upgrade.

chriskim06 and others added 5 commits March 9, 2021 08:24
@chriskim06 @corneliusweig
Update integration_test/install_test.go
f992649 
Co-authored-by: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>
@chriskim06 @corneliusweig
Update integration_test/testutil_test.go
e0d6942 
Co-authored-by: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>
@chriskim06 @corneliusweig
Update integration_test/upgrade_test.go
28e9698 
Co-authored-by: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>
@chriskim06 @corneliusweig
Update integration_test/upgrade_test.go
7cce053 
Co-authored-by: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>
@chriskim06
Cleanup some mistakes from github commit
23a58aa
@ahmetb
Copy link
Member

ahmetb commented Mar 9, 2021

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 9, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahmetb, chriskim06

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit eaf44f4 into kubernetes-sigs:master Mar 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@corneliusweig corneliusweig corneliusweig left review comments

@ahmetb ahmetb Awaiting requested review from ahmetb

Assignees

@ahmetb ahmetb

@corneliusweig corneliusweig

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

creationTimestamp is null in all receipts
4 participants
@chriskim06 @ahmetb @k8s-ci-robot @corneliusweig