-
Notifications
You must be signed in to change notification settings - Fork 49
Restic bug fixes; unit tests and docs formatting #1365
Conversation
There was a problem hiding this 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.
b87f1e2
to
f97f800
Compare
79a1a13
to
bd42db2
Compare
There was a problem hiding this 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!
@@ -31,64 +31,64 @@ Velero component configuration example: | |||
# velero.lokocfg | |||
component "velero" { | |||
|
|||
# provider = "azure/openebs/restic" |
There was a problem hiding this comment.
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.
bd42db2
to
ebd457e
Compare
There was a problem hiding this 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
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>
ebd457e
to
5f0b1e8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this 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.
There was a problem hiding this 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 🚀
8cf20fa
to
03ebe0e
Compare
e3e4af5
to
3cdfbef
Compare
This commit fixes the formatting for the configuration reference documentation of Velero component. Signed-off-by: Imran Pochi <imran@kinvolk.io>
3cdfbef
to
09bf039
Compare
This PR fixes the issue with Restic.Tolerations; adds unit tests and fixes docs formatting.
toleration_seconds
is changed totolerationSeconds
TolerationSeconds
field is now an integer instead of string.TolerationSeconds
is set, thenEffect
must beNoExecute
.