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

🐛 BSLs with validation disabled should be validated at least once #3084

Merged
merged 2 commits into from
Dec 3, 2020
Merged

🐛 BSLs with validation disabled should be validated at least once #3084

merged 2 commits into from
Dec 3, 2020

Conversation

ashish-amarnath
Copy link
Contributor

Signed-off-by: Ashish Amarnath ashisham@vmware.com

@ashish-amarnath ashish-amarnath force-pushed the fix-bsl-freq branch 2 times, most recently from 8f72b85 to 3d308ec Compare November 16, 2020 23:45
@ashish-amarnath ashish-amarnath changed the title [WIP] BSLs with validation disabled should be validated at least once BSLs with validation disabled should be validated at least once Nov 16, 2020
@ashish-amarnath ashish-amarnath added this to In progress in v1.6.0 via automation Nov 16, 2020
@ashish-amarnath ashish-amarnath added this to the v1.6.0 milestone Nov 16, 2020
@ashish-amarnath ashish-amarnath force-pushed the fix-bsl-freq branch 2 times, most recently from 7cad257 to 27b1843 Compare November 17, 2020 00:01
@carlisia
Copy link
Contributor

BSLs with validation disabled should be validated at least once

Why?

@ashish-amarnath
Copy link
Contributor Author

ashish-amarnath commented Nov 17, 2020 •
edited
Loading

Why?

Because without that the rule at https://github.com/vmware-tanzu/velero/blob/main/internal/storage/storagelocation.go#L44 is not satisfied and the status of BSLs whose repeated validation is disabled will never be validated.

// This will always return "true" for the first attempt at validating a location, regardless of its validation frequency setting

And this test that I added will fail without the fix https://github.com/vmware-tanzu/velero/pull/3084/files#diff-dbcfa84383a8788385bd7a7ea6a611ba3608d7b34ff6db6626c28fba93df7662R45-R50

{
	name:                   "should return true when validation frequency is zero and lastValidationTime is nil",
	bslValidationFrequency: &metav1.Duration{Duration: 0},
	defaultLocationInfo: DefaultBackupLocationInfo{
		StoreValidationFrequency: 0,
	},
	ready: true,
},

@ashish-amarnath ashish-amarnath changed the title BSLs with validation disabled should be validated at least once 🐛 BSLs with validation disabled should be validated at least once Nov 18, 2020
carlisia
carlisia suggested changes Nov 18, 2020
View reviewed changes
Copy link
Contributor

@carlisia carlisia left a comment

Choose a reason for hiding this comment

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

Glad you caught this discrepancy!

I made some suggestions/requests.

internal/storage/storagelocation_test.go Outdated Show resolved Hide resolved
internal/storage/storagelocation_test.go Outdated Show resolved Hide resolved
internal/storage/storagelocation_test.go Outdated Show resolved Hide resolved
internal/storage/storagelocation.go Outdated Show resolved Hide resolved
internal/storage/storagelocation.go Outdated Show resolved Hide resolved
internal/storage/storagelocation.go Outdated Show resolved Hide resolved
v1.6.0 automation moved this from In progress to Review in progress Nov 18, 2020
@ashish-amarnath
🐛 BSLs with validation disabled should be validated at least once
1a187fc 
Signed-off-by: Ashish Amarnath <ashisham@vmware.com>
nrb
nrb suggested changes Nov 30, 2020
View reviewed changes
Copy link
Contributor

@nrb nrb left a comment

Choose a reason for hiding this comment

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

Aside from one minor comment change, I think I'm 👍 on this.

nrb
nrb previously approved these changes Nov 30, 2020
View reviewed changes
zubron
zubron previously approved these changes Dec 2, 2020
View reviewed changes
Copy link
Contributor

@zubron zubron left a comment

Choose a reason for hiding this comment

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

Thanks, @ashish-amarnath!

@zubron
Copy link
Contributor

zubron commented Dec 2, 2020

Looks like this needs a final review and approval from @carlisia before merging.

@ashish-amarnath ashish-amarnath dismissed stale reviews from zubron and nrb via 680243d December 3, 2020 04:50
@github-actions github-actions bot requested review from nrb and zubron December 3, 2020 04:50
@ashish-amarnath ashish-amarnath force-pushed the fix-bsl-freq branch 2 times, most recently from b01deb9 to 1f0cf3b Compare December 3, 2020 04:52
@ashish-amarnath
review comments
0d91f0c 
Signed-off-by: Ashish Amarnath <ashisham@vmware.com>
carlisia
carlisia approved these changes Dec 3, 2020
View reviewed changes
Copy link
Contributor

@carlisia carlisia left a comment

Choose a reason for hiding this comment

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

Thank you for this 🙏

v1.6.0 automation moved this from Review in progress to Reviewer approved Dec 3, 2020
@carlisia carlisia merged commit 7727d53 into vmware-tanzu:main Dec 3, 2020
v1.6.0 automation moved this from Reviewer approved to Done Dec 3, 2020
@georgettica georgettica mentioned this pull request Dec 23, 2020
Closed
georgettica pushed a commit to georgettica/velero that referenced this pull request Dec 23, 2020
@ashish-amarnath @georgettica
🐛 BSLs with validation disabled should be validated at least once (vm…
ab9197f 
…ware-tanzu#3084)

* 🐛 BSLs with validation disabled should be validated at least once

Signed-off-by: Ashish Amarnath <ashisham@vmware.com>

* review comments

Signed-off-by: Ashish Amarnath <ashisham@vmware.com>
zubron pushed a commit that referenced this pull request Jan 14, 2021
@ashish-amarnath @zubron
🐛 BSLs with validation disabled should be validated at least once (#3084
f9cc5af 
)

* 🐛 BSLs with validation disabled should be validated at least once

Signed-off-by: Ashish Amarnath <ashisham@vmware.com>

* review comments

Signed-off-by: Ashish Amarnath <ashisham@vmware.com>
georgettica pushed a commit to georgettica/velero that referenced this pull request Jan 26, 2021
@ashish-amarnath @georgettica
🐛 BSLs with validation disabled should be validated at least once (vm…
e6a2c9c 
…ware-tanzu#3084)

* 🐛 BSLs with validation disabled should be validated at least once

Signed-off-by: Ashish Amarnath <ashisham@vmware.com>

* review comments

Signed-off-by: Ashish Amarnath <ashisham@vmware.com>
vadasambar pushed a commit to vadasambar/velero that referenced this pull request Feb 3, 2021
@ashish-amarnath
🐛 BSLs with validation disabled should be validated at least once (vm…
4727bd7 
…ware-tanzu#3084)

* 🐛 BSLs with validation disabled should be validated at least once

Signed-off-by: Ashish Amarnath <ashisham@vmware.com>

* review comments

Signed-off-by: Ashish Amarnath <ashisham@vmware.com>
dharmab pushed a commit to dharmab/velero that referenced this pull request May 25, 2021
@ashish-amarnath @dharmab
🐛 BSLs with validation disabled should be validated at least once (vm…
5114c35 
…ware-tanzu#3084)

* 🐛 BSLs with validation disabled should be validated at least once

Signed-off-by: Ashish Amarnath <ashisham@vmware.com>

* review comments

Signed-off-by: Ashish Amarnath <ashisham@vmware.com>
ywk253100 pushed a commit to ywk253100/velero that referenced this pull request Jun 29, 2021
@ashish-amarnath @ywk253100
🐛 BSLs with validation disabled should be validated at least once (vm…
3e2bf85 
…ware-tanzu#3084)

* 🐛 BSLs with validation disabled should be validated at least once

Signed-off-by: Ashish Amarnath <ashisham@vmware.com>

* review comments

Signed-off-by: Ashish Amarnath <ashisham@vmware.com>
gyaozhou pushed a commit to gyaozhou/velero-read that referenced this pull request May 14, 2022
@ashish-amarnath @gyaozhou
🐛 BSLs with validation disabled should be validated at least once (vm…
4ca97b4 
…ware-tanzu#3084)

* 🐛 BSLs with validation disabled should be validated at least once

Signed-off-by: Ashish Amarnath <ashisham@vmware.com>

* review comments

Signed-off-by: Ashish Amarnath <ashisham@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carlisia carlisia carlisia approved these changes

@zubron zubron zubron approved these changes

@dsu-igeek dsu-igeek Awaiting requested review from dsu-igeek

@nrb nrb Awaiting requested review from nrb

Assignees

@ashish-amarnath ashish-amarnath

Labels
Bug
Projects
No open projects
v1.6.0
  
Done
Milestone
v1.6.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ashish-amarnath @carlisia @zubron @nrb