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

Add missing file licences and do some clean works. #6843

Merged

Conversation

yanggangtony
Copy link
Contributor

@yanggangtony yanggangtony commented Sep 19, 2023

Thank you for contributing to Velero!

Please add a summary of your change

1 add missing file licences
2 fix docs error typo in volume-snapshot-data-movement.md
3 optimize the logs in download_request_controller.go

Does your change fix a particular issue?

Fixes #(issue)

Please indicate you've done the following:

  • Accepted the DCO. Commits without the DCO will delay acceptance.
  • Created a changelog file or added /kind changelog-not-required as a comment on this pull request.
  • Updated the corresponding documentation in site/content/docs/main.

@yanggangtony
Copy link
Contributor Author

/kind changelog-not-required

@github-actions github-actions bot added the kind/changelog-not-required PR does not require a user changelog. Often for docs, website, or build changes label Sep 19, 2023
@@ -169,10 +169,10 @@ func (r *downloadRequestReconciler) Reconcile(ctx context.Context, req ctrl.Requ
Name: backup.Spec.StorageLocation,
}, location); err != nil {
if apierrors.IsNotFound(err) {
log.Errorf("BSL for DownloadRequest cannot be found")
log.Errorf("fail to get BSL for DownloadRequest")
Copy link
Contributor

Choose a reason for hiding this comment

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

here isn't better to have a different message from below how it was?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mistake, thanks review.

sseago
sseago previously approved these changes Sep 19, 2023
@ywk253100
Copy link
Contributor

@yanggangtony Please check the CI failure

@yanggangtony
Copy link
Contributor Author

yanggangtony commented Sep 20, 2023

@yanggangtony Please check the CI failure

Oh, i messup the test code Test_getLastSuccessBySchedule, when continue the schedule is ''
I will fix it later.

@codecov
Copy link

codecov bot commented Sep 20, 2023

Codecov Report

Merging #6843 (1174580) into main (8e01d1b) will not change coverage.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##             main    #6843   +/-   ##
=======================================
  Coverage   60.66%   60.66%           
=======================================
  Files         249      249           
  Lines       26480    26480           
=======================================
  Hits        16065    16065           
  Misses       9270     9270           
  Partials     1145     1145           
Files Coverage Δ
internal/resourcemodifiers/resource_modifiers.go 75.65% <ø> (ø)
.../resourcemodifiers/resource_modifiers_validator.go 100.00% <ø> (ø)
internal/resourcepolicies/resource_policies.go 76.54% <ø> (ø)
internal/resourcepolicies/volume_resources.go 88.79% <ø> (ø)
...nal/resourcepolicies/volume_resources_validator.go 100.00% <ø> (ø)
pkg/controller/download_request_controller.go 57.25% <0.00%> (ø)

ywk253100
ywk253100 previously approved these changes Sep 22, 2023
@@ -1,3 +1,18 @@
/*
Copyright The Velero Contributors.
Copy link
Member

Choose a reason for hiding this comment

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

According to https://www.apache.org/licenses/LICENSE-2.0#apply

Year is required. Thoughts?

Copyright [yyyy] [name of copyright owner]

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 just like the license can be forever.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rebased , add the years.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, according to the project code standards, copyright year is to be omitted.
https://velero.io/docs/v1.12/code-standards/#copyright-header

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure what led to the code standards decision to remove the year. It probably should be there, but the fix to that is to change the code standards (a separate issue from this).

Copy link
Contributor Author

@yanggangtony yanggangtony Sep 29, 2023

Choose a reason for hiding this comment

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

yes, so how to handle this case , revert to last commit? i delete the year? WDYT @sseago

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Follow the code standards guide, delete the year..🥲🥲

@shubham-pampattiwar shubham-pampattiwar merged commit 5ab6672 into vmware-tanzu:main Oct 2, 2023
23 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/changelog-not-required PR does not require a user changelog. Often for docs, website, or build changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants