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 EnvSort struct #631

Merged
merged 1 commit into from
Jun 14, 2017
Merged

Fix EnvSort struct #631

merged 1 commit into from
Jun 14, 2017

Conversation

procrypt
Copy link
Contributor

@procrypt procrypt commented Jun 6, 2017

Fixes #627 and unblocks #518

cc: @surajssd @cdrage

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 6, 2017
@surajssd
Copy link
Member

surajssd commented Jun 6, 2017

Tests(unit tests, functional tests)

cdrage
cdrage requested changes Jun 7, 2017
View reviewed changes
Copy link
Member

@cdrage cdrage left a comment

Choose a reason for hiding this comment

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

I feel that these functions (EnvSort) should be in utils.go as they're used for both OpenShift and Kubernetes.

In regards to the new code in initBuildConfig, I believe that code should be refactored into a new function (perhaps sortEnvVars?).

@procrypt procrypt changed the title Environment variabled are populated in random order Fix EnvSort. Jun 7, 2017
@procrypt procrypt changed the title Fix EnvSort. Fix EnvSort struct Jun 7, 2017
@procrypt procrypt force-pushed the sort_EnvVar_struct branch from 54369f4 to b89eb54 Compare June 12, 2017 06:47
@procrypt procrypt mentioned this pull request Jun 12, 2017
Closed
@procrypt procrypt force-pushed the sort_EnvVar_struct branch from b89eb54 to 13bb581 Compare June 12, 2017 10:27
@procrypt
Copy link
Contributor Author

Ping @kadel please review.

kadel
kadel reviewed Jun 12, 2017
View reviewed changes
Copy link
Member

@kadel kadel left a comment

Choose a reason for hiding this comment

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

except adding one small comment, LGTM

pkg/transformer/kubernetes/kubernetes.go Outdated
for _, v := range service.Environment {
envs = append(envs, api.EnvVar{
Name: v.Name,
Value: v.Value,
})
}

sort.Sort(envs)
Copy link
Member

Choose a reason for hiding this comment

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

Can you add please add a short comment explaining why it needs to be sorted?

@cdrage
Copy link
Member

cdrage commented Jun 12, 2017 •
edited
Loading

LGTM / I approve these changes. Other than the nitpick from @kadel 's comment. I agree with adding a few comments.

@procrypt procrypt force-pushed the sort_EnvVar_struct branch from 13bb581 to 719dae9 Compare June 14, 2017 07:29
@procrypt procrypt mentioned this pull request Jun 14, 2017
Merged
@kadel kadel merged commit bf20029 into kubernetes:master Jun 14, 2017
@procrypt
Copy link
Contributor Author

Thanks @kadel ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kadel kadel kadel left review comments

@cdrage cdrage cdrage approved these changes

Assignees
No one assigned
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@procrypt @surajssd @cdrage @kadel @k8s-ci-robot