Skip to content
This repository has been archived by the owner on Jun 29, 2022. It is now read-only.

Restic bug fixes; unit tests and docs formatting #1365

Merged
merged 6 commits into from
Feb 11, 2021
Merged

Conversation

ipochi
Copy link
Member

@ipochi ipochi commented Feb 8, 2021

This PR fixes the issue with Restic.Tolerations; adds unit tests and fixes docs formatting.

  • json tag toleration_seconds is changed to tolerationSeconds
  • TolerationSeconds field is now an integer instead of string.
  • Conditional checks added to ensure if TolerationSeconds is set, then Effect must be NoExecute.
  • Adds unit tests
  • Fixes formatting of Velero component reference documentation.

pkg/components/velero/restic/restic.go Outdated Show resolved Hide resolved
pkg/components/velero/restic/restic.go Show resolved Hide resolved
pkg/components/util/types.go Show resolved Hide resolved
pkg/components/velero/component_test.go Outdated Show resolved Hide resolved
pkg/components/velero/restic/restic_test.go Outdated Show resolved Hide resolved
pkg/components/velero/restic/restic_test.go Outdated Show resolved Hide resolved
Copy link
Member

@invidian invidian left a comment

Choose a reason for hiding this comment

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

Forgot to mention, unit tests should be added together with new code in a single commit, to keep things atomic.

@ipochi ipochi force-pushed the imran/restic-fixes branch 2 times, most recently from 79a1a13 to bd42db2 Compare February 9, 2021 05:28
Copy link
Member

@invidian invidian 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 adding commit messages @ipochi, think make more sense now!

pkg/components/velero/component_test.go Show resolved Hide resolved
pkg/components/velero/component_test.go Show resolved Hide resolved
pkg/components/velero/component_test.go Show resolved Hide resolved
pkg/components/velero/component_test.go Show resolved Hide resolved
pkg/components/velero/restic/restic.go Outdated Show resolved Hide resolved
pkg/components/velero/restic/restic_test.go Show resolved Hide resolved
@@ -31,64 +31,64 @@ Velero component configuration example:
# velero.lokocfg
component "velero" {

# provider = "azure/openebs/restic"
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I suggest opening separate PRs with such fixes, so they can be immediately merged.

Copy link
Member

@knrt10 knrt10 left a comment

Choose a reason for hiding this comment

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

Small NITs, rest LGTM

pkg/components/velero/component_test.go Show resolved Hide resolved
pkg/components/velero/restic/restic.go Outdated Show resolved Hide resolved
Signed-off-by: Imran Pochi <imran@kinvolk.io>
This commit updates the TolerationSeconds from string to int64.
Also update the json tag from `toleration_seconds` to `tolerationSeconds`.

JSON tag is updated because, Tolerations struct is directly marshalled to
JSON and stored as a string TolerationsRaw which is then passed to the
helm templates. This causes the Kubernetes API Server to complain during
validation that no such field regarding `toleration_seconds` since API
server was expected `tolerationSeconds`.

Data type is changed to int64, becuase Kubernetes API server expects the
TolerationSeconds to be int64 instead of string. Thereby causing
validation to fail at the API server end.

Updates the unit test to reflect the same.

Signed-off-by: Imran Pochi <imran@kinvolk.io>
This commit adds validation to the Tolerations type.

This is done so because Kubernetes API server expects that if
`TolerationSeconds` is set then `Effect` must be set to `NoExecute`.
This condition checks for the same and reports and error to the user if
this condition is not met.

Adds unit test for the same.

Signed-off-by: Imran Pochi <imran@kinvolk.io>
This commit adds a check to check if restic.tolerations.toleration_seconds is
set then the value of restic.tolerations.effect must be `NoExecute`

Signed-off-by: Imran Pochi <imran@kinvolk.io>
This commit adds a conditional check for restic.region is to be set if
restic.provider is `aws`.

This change is done to reflect the addtional configuration of region
that is required when provider is `aws`. Without this additional check
lokoctl throws an error message that is caught few levels below in the
stack in the velero. Catching this error allows a consistent error
message reporting to the user.

If any other provider is used, then region value is optional.

Updates the unit test to reflect the same.

Signed-off-by: Imran Pochi <imran@kinvolk.io>
knrt10
knrt10 previously approved these changes Feb 11, 2021
Copy link
Member

@knrt10 knrt10 left a comment

Choose a reason for hiding this comment

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

+1

Copy link
Member

@surajssd surajssd left a comment

Choose a reason for hiding this comment

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

Just one comment on making it a method instead of a function would benefit other users of toleration types.

Scratch that. I see that the function is in the library.

surajssd
surajssd previously approved these changes Feb 11, 2021
invidian
invidian previously approved these changes Feb 11, 2021
Copy link
Member

@invidian invidian left a comment

Choose a reason for hiding this comment

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

LGTM, nice work @ipochi 🚀

@ipochi ipochi dismissed stale reviews from invidian, surajssd, and knrt10 via 8cf20fa February 11, 2021 10:02
@ipochi ipochi force-pushed the imran/restic-fixes branch 2 times, most recently from 8cf20fa to 03ebe0e Compare February 11, 2021 10:48
@ipochi ipochi force-pushed the imran/restic-fixes branch 2 times, most recently from e3e4af5 to 3cdfbef Compare February 11, 2021 13:57
This commit fixes the formatting for the configuration reference
documentation of Velero component.

Signed-off-by: Imran Pochi <imran@kinvolk.io>
@ipochi ipochi merged commit e5d1bd6 into master Feb 11, 2021
@ipochi ipochi deleted the imran/restic-fixes branch February 11, 2021 16:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants