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

Fix OADP 526 #704

Merged
merged 2 commits into from
May 24, 2022
Merged

Fix OADP 526 #704

merged 2 commits into from
May 24, 2022

Conversation

shubham-pampattiwar
Copy link
Member

@openshift-ci openshift-ci bot requested a review from rayfordj May 19, 2022 14:26
pranavgaikwad
pranavgaikwad requested changes May 19, 2022
View reviewed changes
Copy link

@pranavgaikwad pranavgaikwad left a comment

Choose a reason for hiding this comment

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

It'd be nice to have a test for invalid resource inputs. Otherwise looks great. Thanks for quick turnaround!

controllers/restic.go Show resolved Hide resolved
controllers/restic_test.go Show resolved Hide resolved
controllers/validator.go Show resolved Hide resolved
controllers/velero.go
ResourcesReqs.Requests[corev1.ResourceMemory] = resource.MustParse(dpa.Spec.Configuration.Restic.PodConfig.ResourceAllocations.Requests.Memory().String())
// Set custom limits and requests values if defined on Restic Spec
if dpa.Spec.Configuration.Restic.PodConfig.ResourceAllocations.Requests.Cpu() != nil && dpa.Spec.Configuration.Restic.PodConfig.ResourceAllocations.Requests.Cpu().Value() != 0 {
parsedQuantity, err := resource.ParseQuantity(dpa.Spec.Configuration.Restic.PodConfig.ResourceAllocations.Requests.Cpu().String())
Copy link

Choose a reason for hiding this comment

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

+1 to using ParseQuantity()

kaovilai
kaovilai reviewed May 19, 2022
View reviewed changes
Copy link
Member

@kaovilai kaovilai left a comment

Choose a reason for hiding this comment

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

+1

@pranavgaikwad
Copy link

I just realized that since resource is a typed field. Validation on the CR itself will kick in and prevent users from putting stupid values. @shubham-pampattiwar approving

@shubham-pampattiwar
Copy link
Member Author

/retest

1 similar comment
@shubham-pampattiwar
Copy link
Member Author

/retest

@openshift-ci
Copy link

openshift-ci bot commented May 19, 2022

@shubham-pampattiwar: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@dymurray dymurray merged commit e500ddf into openshift:master May 24, 2022
@kaovilai
Copy link
Member

kaovilai commented May 26, 2022 •
edited by openshift-ci bot

From OADP-526 Fix version says 1.0.3
/cherrypick oadp-1.0

@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request May 26, 2022
Merged
@openshift-cherrypick-robot
Copy link
Contributor

openshift-cherrypick-robot commented May 26, 2022 •
edited by openshift-ci bot

@kaovilai: new pull request created: #712

In response to this:

From OADP-526 Fix version says 1.0.3
/cherrypick oadp-1.0

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

kaovilai pushed a commit to kaovilai/oadp-operator that referenced this pull request Jun 1, 2022
@shubham-pampattiwar @kaovilai
Fix OADP 526 (openshift#704)
7ea5b67 
* fix OADP 526

* add ignore err comments
kaovilai added a commit that referenced this pull request Jun 3, 2022
@kaovilai @shubham-pampattiwar
@savitharaghunathan @jmontleon @dymurray
Remove NoDefaultBackupLocation e2e entry + registry probe timeout bum…
09024d8 
…p (fix azure flakes) (#717)

* revert to b64e96e

* bump probe to 10s

* mimic velero cli loglevel options (#692)

use logrus ParseLevel directly

* Fix OADP 526 (#704)

* fix OADP 526

* add ignore err comments

* fix docs link for build from src (#706)

* Datamover CRD design (#597)

* Adding initial datamover design

* Typo in reviewers

* Add summary & user stories

* Fixing nits

* Adding intial feedback

* Adding feedback

* nit: changing the selector field name

* Adding review comments

* Adding plugin clarification

* updating image to reflect the latest design

* Reflect recent velero csi plugin changes

* use a full vm for arm builds in travis (#713)

* OADP-535 allow for nullable resource allocations (#711)

* OADP-535 allow for nullable resource allocations

* Add missing nullables

* Include nullable on additionalProperties

Co-authored-by: Shubham Pampattiwar <shubhampampattiwar7@gmail.com>
Co-authored-by: Savitha Raghunathan <sraghuna@redhat.com>
Co-authored-by: Jason Montleon <jmontleo@redhat.com>
Co-authored-by: Dylan Murray <dymurray@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pranavgaikwad pranavgaikwad pranavgaikwad approved these changes

@kaovilai kaovilai kaovilai approved these changes

@dymurray dymurray Awaiting requested review from dymurray

@rayfordj rayfordj Awaiting requested review from rayfordj

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@shubham-pampattiwar @pranavgaikwad @kaovilai @openshift-cherrypick-robot @dymurray