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

chore: cherry-pick #625 to 7.52.x #634

Closed
wants to merge 2 commits into from

Conversation

AObuchow
Copy link

What does this PR do?

Add changes from #625 into the 7.52.x branch.

This back port led to some conflicts, so hopefully I resolved them properly. If not, feel free to either modify this PR or discard it and redo it yourself.

What issues does this PR fix or reference?

eclipse-che/che#21405

Is it tested? How?

I ran the automated tests with npm run test

@che-bot
Copy link
Contributor

che-bot commented Sep 19, 2022

Click here to review and test in web IDE: Contribute

@openshift-ci
Copy link

openshift-ci bot commented Sep 19, 2022

Hi @AObuchow. Thanks for your PR.

I'm waiting for a eclipse-che member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@nickboldt nickboldt self-requested a review September 19, 2022 20:07
Copy link
Contributor

@nickboldt nickboldt left a comment

Choose a reason for hiding this comment

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

So that the commit log is more clear about blockers fixed for the product, can you update your commit message so it refers to the original commit message? Add external attributes to workspaces is much more descriptive than cherrypick #625, and will make it immediately obvious when checking the commit log that a fix was ported (and especially WHY).

@AObuchow
Copy link
Author

So that the commit log is more clear about blockers fixed for the product, can you update your commit message so it refers to the original commit message? Add external attributes to workspaces is much more descriptive than cherrypick #625, and will make it immediately obvious when checking the commit log that a fix was ported (and especially WHY).

Sounds good to me - will update it shortly :)

@che-bot
Copy link
Contributor

che-bot commented Sep 19, 2022

Click here to review and test in web IDE: Contribute

@nickboldt nickboldt self-requested a review September 19, 2022 20:39
Copy link
Contributor

@nickboldt nickboldt left a comment

Choose a reason for hiding this comment

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

LGTM (in terms of commit message metadata) but not an SME so don't trust me here... especially with the comment above about that this cherrypick broke stuff and wasn't completely straightforward.

@openshift-ci openshift-ci bot added the lgtm label Sep 19, 2022
@openshift-ci openshift-ci bot removed the lgtm label Sep 20, 2022
@che-bot
Copy link
Contributor

che-bot commented Sep 20, 2022

Click here to review and test in web IDE: Contribute

1 similar comment
@che-bot
Copy link
Contributor

che-bot commented Sep 20, 2022

Click here to review and test in web IDE: Contribute

@olexii4 olexii4 changed the title chore: cherry-pick #625 to 7.52.x [wip]chore: cherry-pick #625 to 7.52.x Sep 20, 2022
@nickboldt
Copy link
Contributor

dumb question.... do we need to run tests with node 14? it's EOL soon and 16 is the LTS version

https://endoflife.date/nodejs

@che-bot
Copy link
Contributor

che-bot commented Sep 21, 2022

Click here to review and test in web IDE: Contribute

@che-bot
Copy link
Contributor

che-bot commented Sep 21, 2022

Click here to review and test in web IDE: Contribute

Allows for injecting storage-type and external config attributes into devworkspaces

Signed-off-by: Andrew Obuchowicz <[email protected]>
@che-bot
Copy link
Contributor

che-bot commented Sep 21, 2022

Click here to review and test in web IDE: Contribute

@olexii4 olexii4 changed the title [wip]chore: cherry-pick #625 to 7.52.x chore: cherry-pick #625 to 7.52.x Sep 21, 2022
@openshift-ci
Copy link

openshift-ci bot commented Sep 21, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: AObuchow, nickboldt, olexii4

The full list of commands accepted by this bot can be found 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

* fix: storage type select widget

Signed-off-by: Oleksii Orel <[email protected]>
@openshift-ci openshift-ci bot removed the lgtm label Sep 21, 2022
@openshift-ci
Copy link

openshift-ci bot commented Sep 21, 2022

New changes are detected. LGTM label has been removed.

@che-bot
Copy link
Contributor

che-bot commented Sep 21, 2022

Click here to review and test in web IDE: Contribute

@nickboldt
Copy link
Contributor

the PR checks will never pass because they require docker login to quay.io, which uses secrets that will never be accessible to @AObuchow 's fork, only this repo.

@nickboldt
Copy link
Contributor

Closing in favour of #635

@nickboldt nickboldt closed this Sep 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants