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

Bug 1782726: Fixed PVC creation and GCP issues #3677

Merged
merged 1 commit into from Dec 12, 2019
Merged

Bug 1782726: Fixed PVC creation and GCP issues #3677

merged 1 commit into from Dec 12, 2019

Conversation

bipuladh
Copy link
Contributor

@bipuladh bipuladh commented Dec 5, 2019

  • Added missing field for GCP target bucket
  • Added missing help text for GCP upload
  • Fixes payload for GCP and S3 Endpoint Type

@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 5, 2019
@openshift-ci-robot openshift-ci-robot added the component/noobaa Related to noobaa-storage-plugin label Dec 5, 2019
@gnehapk
Copy link
Contributor

gnehapk commented Dec 5, 2019

Please add bugzilla id to this PR

gnehapk
gnehapk reviewed Dec 5, 2019
View reviewed changes
frontend/packages/noobaa-storage-plugin/src/components/create-backingstore-page/create-bs.tsx
};
if (provider === 'PVC') {
// eslint-disable-next-line
bsPayload.spec['pvPool'] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

why not bsPayload.spec.pvPool? Then you don't need disable eslint for next line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it dont work like that. Have a look at spec object.

Copy link
Contributor

Choose a reason for hiding this comment

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

ack

gnehapk
gnehapk reviewed Dec 5, 2019
View reviewed changes
frontend/packages/noobaa-storage-plugin/src/components/create-backingstore-page/create-bs.tsx
}
if (provider === 'S3 Compatible') {
// eslint-disable-next-line
bsPayload.spec['s3Compatible'] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as above

gnehapk
gnehapk reviewed Dec 5, 2019
View reviewed changes
frontend/packages/noobaa-storage-plugin/src/components/create-backingstore-page/create-bs.tsx
// eslint-disable-next-line
bsPayload.spec['s3Compatible'] = {
// eslint-disable-next-line
...bsPayload.spec['s3Compatible'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as above

gnehapk
gnehapk reviewed Dec 5, 2019
View reviewed changes
frontend/packages/noobaa-storage-plugin/src/components/create-backingstore-page/create-bs.tsx Outdated

};
}
if (provider === 'S3 Compatible') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Move all these strings to constants and the use here. Same applies to rest of the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack

gnehapk
gnehapk reviewed Dec 5, 2019
View reviewed changes
frontend/packages/noobaa-storage-plugin/src/components/create-backingstore-page/create-bs.tsx Outdated
endpoint: providerDataState.endpoint,
};
}
// Add region in the end
if (provider === 'AWS S3') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Move it to constant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack

afreen23
afreen23 reviewed Dec 9, 2019
View reviewed changes
frontend/packages/noobaa-storage-plugin/src/components/create-backingstore-page/create-bs.tsx Outdated
};

const typeNoobaaMap = {
'AWS S3': 'aws-s3',
'S3 Compatible': 's3-compatible',
'Azure Blob': 'azure-blob',
'Google cloud storage': 'google-cloud-storage',
Copy link
Contributor

Choose a reason for hiding this comment

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

freeze this object and name in caps

Copy link
Contributor

Choose a reason for hiding this comment

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

You have enum BC_PROVIDERS.
The key values can be constructed from that to track changes at one place only.

frontend/packages/noobaa-storage-plugin/src/components/create-backingstore-page/create-bs.tsx Outdated
@@ -58,12 +59,14 @@ const bucketNoobaaMap = {
'AWS S3': 'targetBucket',
'S3 Compatible': 'targetBucket',
'Azure Blob': 'targetBlobContainer',
'Google cloud storage': 'targetBucket',
Copy link
Contributor

Choose a reason for hiding this comment

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

You have enum BC_PROVIDERS.
The key values can be constructed from that to track changes at one place only.

afreen23
afreen23 reviewed Dec 10, 2019
View reviewed changes
frontend/packages/noobaa-storage-plugin/src/constants/index.ts Outdated
AWS = 'AWS S3',
S3 = 'S3 Compatible',
PVC = 'PVC',
GOOG = 'Google cloud storage',
Copy link
Contributor

Choose a reason for hiding this comment

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

A curiosity question, why GOOG and not GCS or GCP?
(Though I saw GCP is used explicitly at various places in code)

@bipuladh
Copy link
Contributor Author

/retest

@afreen23
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 10, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afreen23, bipuladh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

7 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@bipuladh
Copy link
Contributor Author

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

3 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@afreen23
Copy link
Contributor

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

12 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@bipuladh
Copy link
Contributor Author

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit c4cb362 into openshift:master Dec 12, 2019
@bipuladh bipuladh changed the title Fixed PVC creation and GCP issues Bug 1782726: Fixed PVC creation and GCP issues Dec 12, 2019
@openshift-ci-robot
Copy link
Contributor

@bipuladh: All pull requests linked via external trackers have merged. Bugzilla bug 1782726 has been moved to the MODIFIED state.

In response to this:

Bug 1782726: Fixed PVC creation and GCP issues

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.

@bipuladh
Copy link
Contributor Author

/cherrypick release-4.3

@openshift-cherrypick-robot
Copy link

@bipuladh: #3677 failed to apply on top of branch "release-4.3":

error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	frontend/packages/noobaa-storage-plugin/src/components/create-backingstore-page/create-bs-page.tsx
M	frontend/packages/noobaa-storage-plugin/src/components/create-backingstore-page/create-bs.tsx
Falling back to patching base and 3-way merge...
Auto-merging frontend/packages/noobaa-storage-plugin/src/components/create-backingstore-page/create-bs.tsx
CONFLICT (content): Merge conflict in frontend/packages/noobaa-storage-plugin/src/components/create-backingstore-page/create-bs.tsx
Auto-merging frontend/packages/noobaa-storage-plugin/src/components/create-backingstore-page/create-bs-page.tsx
CONFLICT (content): Merge conflict in frontend/packages/noobaa-storage-plugin/src/components/create-backingstore-page/create-bs-page.tsx
Patch failed at 0001 Fixed PVC creation and GCP issues

In response to this:

/cherrypick release-4.3

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.

@bipuladh bipuladh mentioned this pull request Dec 12, 2019
Merged
@spadgett spadgett added this to the v4.4 milestone Dec 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gnehapk gnehapk gnehapk left review comments

@afreen23 afreen23 afreen23 left review comments

@a2batic a2batic Awaiting requested review from a2batic

@cloudbehl cloudbehl Awaiting requested review from cloudbehl

Assignees

@afreen23 afreen23

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/noobaa Related to noobaa-storage-plugin lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v4.4
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@bipuladh @gnehapk @afreen23 @openshift-ci-robot @openshift-bot @openshift-cherrypick-robot @spadgett @openshift-merge-robot