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

Additional volume options #161

Merged
merged 6 commits into from
Aug 7, 2020
Merged

Conversation

eytan-avisror
Copy link
Collaborator

Fixes #155

Adds additional volume options to NodeVolumes

@eytan-avisror eytan-avisror requested review from a team as code owners August 7, 2020 20:57
@eytan-avisror eytan-avisror requested a review from vgunapati August 7, 2020 20:57
@codecov
Copy link

codecov bot commented Aug 7, 2020

Codecov Report

Merging #161 into master will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #161   +/-   ##
=======================================
  Coverage   89.54%   89.54%           
=======================================
  Files          13       13           
  Lines        1607     1607           
=======================================
  Hits         1439     1439           
  Misses        111      111           
  Partials       57       57           
Impacted Files Coverage Δ
controllers/provisioners/eks/helpers.go 90.09% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa48faa...e20c922. Read the comment docs.

Copy link

@shaoxt shaoxt left a comment

Choose a reason for hiding this comment

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

This feature is to gather IOPS, DeleteonTermination and Encrypted info from AWS right ?

@eytan-avisror
Copy link
Collaborator Author

This feature is to gather IOPS, DeleteonTermination and Encrypted info from AWS right ?

It's to allow these fields to be configurable in the custom resource

@shaoxt
Copy link

shaoxt commented Aug 7, 2020

OK, what if the old configuration doesn't have UserDataStage/data or UserDataStage/stage ?
Will it causes failure or the validation could take care of the case ?

This feature is to gather IOPS, DeleteonTermination and Encrypted info from AWS right ?

It's to allow these fields to be configurable in the custom resource

@eytan-avisror
Copy link
Collaborator Author

eytan-avisror commented Aug 7, 2020

OK, what if the old configuration doesn't have UserDataStage/data or UserDataStage/stage ?
Will it causes failure or the validation could take care of the case ?

This feature is to gather IOPS, DeleteonTermination and Encrypted info from AWS right ?

It's to allow these fields to be configurable in the custom resource

The UserDataStage type changes are backwards compatible, we only added a "name" field and this PR also makes it 'omitempty' to make sure if an old CR did not have this field it won't break.
The name field we added is for convenience anyway and is not mandatory

we did make the other two fields (stage, data) required, because without them it would fail anyway

Copy link
Contributor

@vgunapati vgunapati left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@shaoxt shaoxt left a comment

Choose a reason for hiding this comment

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

LGTM

@eytan-avisror eytan-avisror merged commit 4156065 into keikoproj:master Aug 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow IOPS config option for io1 volumeType
4 participants