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

Make variable expansion work for environment variables in k8s #12689

Merged
merged 3 commits into from
Feb 25, 2019
Merged

Make variable expansion work for environment variables in k8s #12689

merged 3 commits into from
Feb 25, 2019

Conversation

metlos
Copy link
Contributor

@metlos metlos commented Feb 18, 2019

What does this PR do?

K8s does the expansion only if it already knows about the variable being
expanded.

This means we have to sort the environment variable list prior to sending
it to k8s in such a way that vars that reference others always follow the
referenced ones.

This is a reimplementation of #12610 that uses an appropriate algorithm to produce a consistent sorting of the env vars.

What issues does this PR fix or reference?

#12577

Release Notes

  • TODO

Docs PR

N/A

@che-bot che-bot added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/enhancement A feature request - must adhere to the feature request template. labels Feb 18, 2019
@metlos
Copy link
Contributor Author

metlos commented Feb 18, 2019

ci-test

@metlos metlos self-assigned this Feb 18, 2019
@che-bot
Copy link
Contributor

che-bot commented Feb 18, 2019

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:12689
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@metlos
Copy link
Contributor Author

metlos commented Feb 19, 2019

ci-build

@metlos
Copy link
Contributor Author

metlos commented Feb 19, 2019

ci-test

@metlos metlos marked this pull request as ready for review February 19, 2019 11:51
@che-bot
Copy link
Contributor

che-bot commented Feb 19, 2019

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:12689
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@metlos
Copy link
Contributor Author

metlos commented Feb 20, 2019

ci-test

@che-bot
Copy link
Contributor

che-bot commented Feb 20, 2019

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:12689
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@metlos
Copy link
Contributor Author

metlos commented Feb 21, 2019

ci-test

@che-bot
Copy link
Contributor

che-bot commented Feb 21, 2019

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:12689
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@metlos
Copy link
Contributor Author

metlos commented Feb 21, 2019

@sleshchenko @garagatyi could you please take a look and review this?

sleshchenko
sleshchenko approved these changes Feb 22, 2019
View reviewed changes
Copy link
Member

@sleshchenko sleshchenko left a comment

Choose a reason for hiding this comment

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

Good job 👍

Really great javadocs and comments that help understand the implementation

@metlos
Copy link
Contributor Author

metlos commented Feb 22, 2019

ci-test

@che-bot
Copy link
Contributor

che-bot commented Feb 22, 2019

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:12689
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@metlos
Copy link
Contributor Author

metlos commented Feb 22, 2019

I squashed and rebased on top of latest master to clean up the PR a little

@metlos
Copy link
Contributor Author

metlos commented Feb 25, 2019

Squashed commits together before merging.

garagatyi
garagatyi reviewed Feb 25, 2019
View reviewed changes
Copy link

@garagatyi garagatyi left a comment

Choose a reason for hiding this comment

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

It seems I failed to understand the algorithm to the end. Can't approve or requests changes because of that. I left some comments, but rely on other reviewers reviews on whether to merge right away.

...ommons/che-core-commons-lang/src/main/java/org/eclipse/che/commons/lang/TopologicalSort.java Outdated Show resolved Hide resolved
...ommons/che-core-commons-lang/src/main/java/org/eclipse/che/commons/lang/TopologicalSort.java Outdated Show resolved Hide resolved
...ommons/che-core-commons-lang/src/main/java/org/eclipse/che/commons/lang/TopologicalSort.java Show resolved Hide resolved
metlos and others added 2 commits February 25, 2019 13:18
@metlos
Make variable expansion work for environment variables in k8s.
f66e967 
K8s does the expansion only if it already knows about the variable being
expanded.

This means we have to sort the environment variable list prior to sending
it to k8s in such a way that vars that reference others always follow the
referenced ones.

Signed-off-by: Lukas Krejci <lkrejci@redhat.com>
@metlos
Fix incorrect wording in the comments
53787f0 
Signed-off-by: Lukas Krejci <lkrejci@redhat.com>
@metlos
* Don't try to remove already removed element during removeFirstIndep…
9a8a4f8 
…endent

* Presize the LinkedHashMap correctly

Signed-off-by: Lukas Krejci <lkrejci@redhat.com>
@metlos
Copy link
Contributor Author

metlos commented Feb 25, 2019

ci-build

@metlos metlos merged commit 88da873 into eclipse-che:master Feb 25, 2019
@che-bot che-bot removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Feb 25, 2019
@che-bot che-bot added this to the 6.19.0 milestone Feb 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@garagatyi garagatyi garagatyi left review comments

@sleshchenko sleshchenko sleshchenko approved these changes

@skabashnyuk skabashnyuk skabashnyuk approved these changes

Assignees

@metlos metlos

Labels
kind/enhancement A feature request - must adhere to the feature request template.
Projects
None yet
Milestone
6.19.0
Development

Successfully merging this pull request may close these issues.
5 participants
@metlos @che-bot @skabashnyuk @garagatyi @sleshchenko