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

fix: handle env-vars case sensitive #1493

Merged
merged 1 commit into from
Dec 7, 2022
Merged

fix: handle env-vars case sensitive #1493

merged 1 commit into from
Dec 7, 2022

Conversation

KnisterPeter
Copy link
Member

Closes #1488

@KnisterPeter KnisterPeter requested a review from a team as a code owner December 7, 2022 11:16
@KnisterPeter KnisterPeter self-assigned this Dec 7, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Dec 7, 2022

🦙 MegaLinter status: ✅ SUCCESS

Descriptor Linter Files Fixed Errors Elapsed time
✅ EDITORCONFIG editorconfig-checker 2 0 0.01s
✅ REPOSITORY gitleaks yes no 2.49s
✅ REPOSITORY git_diff yes no 0.0s
✅ REPOSITORY secretlint yes no 0.9s

See errors details in artifact MegaLinter reports on CI Job page
Set VALIDATE_ALL_CODEBASE: true in mega-linter.yml to validate all sources, not only the diff

MegaLinter is graciously provided by OX Security

@codecov
Copy link

codecov bot commented Dec 7, 2022

Codecov Report

Merging #1493 (c29694a) into master (4f8da0a) will increase coverage by 3.68%.
The diff coverage is 67.46%.

@@            Coverage Diff             @@
##           master    #1493      +/-   ##
==========================================
+ Coverage   57.50%   61.19%   +3.68%     
==========================================
  Files          32       45      +13     
  Lines        4594     6957    +2363     
==========================================
+ Hits         2642     4257    +1615     
- Misses       1729     2400     +671     
- Partials      223      300      +77     
Impacted Files Coverage Δ
pkg/common/file.go 0.00% <0.00%> (ø)
pkg/container/docker_logger.go 52.08% <ø> (ø)
pkg/container/host_environment.go 0.00% <0.00%> (ø)
pkg/container/parse_env_file.go 0.00% <0.00%> (ø)
pkg/container/util.go 0.00% <0.00%> (ø)
pkg/model/action.go 0.00% <0.00%> (ø)
pkg/model/step_result.go 0.00% <ø> (ø)
pkg/container/docker_run.go 13.58% <11.48%> (+8.04%) ⬆️
...ontainer/linux_container_environment_extensions.go 24.32% <24.32%> (ø)
pkg/model/workflow.go 46.43% <25.25%> (-4.48%) ⬇️
... and 38 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jsoref
Copy link
Contributor

jsoref commented Dec 7, 2022

Technically they're case-preserving on Windows and case-sensitive everywhere else.

@ChristopherHX
Copy link
Contributor

Yes, but it requires a mass change to replace the golang map object with a custom interface.

Copy link
Contributor

@ChristopherHX ChristopherHX left a comment

Choose a reason for hiding this comment

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

This basically just reverts a previous change and adds a test.

@jsoref
Copy link
Contributor

jsoref commented Dec 7, 2022

Right. As long as we're generally evolving to match GHA and people understand that what is being done here isn't the final state.

@KnisterPeter
Copy link
Member Author

@jsoref I do understand the difference but what do you see as limitation with the current implementation? We are probably not converting case of env-var somewhere.
The only issue I see would be that a user sets an env-var with a different case then an existing one and expects it to be overwritten. Is this what you are aiming at?

Feel free to open PRs to improve on this topic.

@jsoref
Copy link
Contributor

jsoref commented Dec 7, 2022

On Windows, yes, that.

Note: I don't really use Windows for my GHA stuff. I just dabble in it around the edges and know most of the quirks (and have filed bugs about the GHA docs which were wrong on these same points).

I'm just slightly worried that people will assume that the tests and commit descriptions define normative behavior for all platforms where as they only define the closest-desired behavior for Linux (and macOS).

@mergify mergify bot merged commit 0f8861b into master Dec 7, 2022
@mergify mergify bot deleted the fix-env-vars-casing branch December 7, 2022 15:31
@catthehacker
Copy link
Member

I'm just slightly worried that people will assume that the tests and commit descriptions define normative behavior for all platforms where as they only define the closest-desired behavior for Linux (and macOS).

They shouldn't trust any software then...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

environment variables should be case sensitive
4 participants