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

Support envs on external render commands #5278

Merged
merged 2 commits into from
Nov 20, 2018

Conversation

lunny
Copy link
Member

@lunny lunny commented Nov 6, 2018

An improvement for #5201. @Cellebyte

@lunny lunny added the type/enhancement An improvement of existing functionality label Nov 6, 2018
@lunny lunny added this to the 1.7.0 milestone Nov 6, 2018
@codecov-io
Copy link

codecov-io commented Nov 6, 2018

Codecov Report

Merging #5278 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5278      +/-   ##
==========================================
- Coverage   37.42%   37.42%   -0.01%     
==========================================
  Files         312      312              
  Lines       46480    46480              
==========================================
- Hits        17394    17393       -1     
  Misses      26596    26596              
- Partials     2490     2491       +1
Impacted Files Coverage Δ
modules/process/manager.go 76.81% <0%> (-4.35%) ⬇️
models/repo_list.go 64.55% <0%> (+1.26%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8ef177f...0d9034e. Read the comment docs.

@bkcsoft bkcsoft added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 6, 2018
@bkcsoft bkcsoft added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Nov 6, 2018
@zeripath
Copy link
Contributor

zeripath commented Nov 8, 2018

Hmm... I'm not sure there should be differential expansion based on whether the machine is running windows or otherwise. os.Expand and os.ExpandEnv only do $var and ${var} by design. (This PR should also probably cover ${...} expansion too.)

I guess we only want GITEA_PREFIX_SRC, GITEA_PREFIX_RAW expanded at present but it may be better to restructure this to use os.Expand.

(In passing I'm also quite suspicious of the use of strings.Field. It could be a bit of a gotcha to want an external command which passes an argument -c "Gitea made this". This will get munched into: ["-c", "\"Gitea", "made", "this\""] which will effectively tell the command that the argument to the -c is \"Gitea not Gitea made this. I'd have to check though.)

@lunny
Copy link
Member Author

lunny commented Nov 9, 2018

@zeripath see golang/go#24848, os.Expand didn't support windows and also wont do that. We could use golang.org/x/sys/windows/registry when windows but I would like to write the function myself since it's a function too short.

@zeripath
Copy link
Contributor

zeripath commented Nov 9, 2018

It's not that os.Expand doesn't support Windows, but that it won't support Windows-style variables. Currently we're not doing any expansion of variables, so the choice is to support different variable expansion regimes for Windows vs. others or just to do the same.

Hmm... Arguments in favour of 'doze expansion:

  • The other arguments in the command be passed to the windows command unchanged - therefore paths referred to in the command will have to be backslash file-separated.
  • It matches the expected format for the command knowing that they're on windows

Arguments against specific 'doze expansion:

  • The variable format will be different on different systems.

I'm sorry, I think you're right we should do 'doze expansion.

(I've just checked go's documentation, is my interpretation right in that the filepath for the command would have to be in 'doze format with backslashes? e.g. "C:\Program Files (x86)\Why so many spaces\command.exe" - if not that probably needs looking at too.)

It might still be worth handling ${..} expansion. Are there other variables we might want to allow expansion for, or is it just these two?

@lunny
Copy link
Member Author

lunny commented Nov 20, 2018

@zeripath currently just these two and I don't think we need to support ${..} because it's not an unix/windows environment variables syntax.

@bkcsoft bkcsoft added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Nov 20, 2018
@techknowlogick techknowlogick merged commit cef0f12 into go-gitea:master Nov 20, 2018
@lunny lunny deleted the lunny/third_render_envs branch November 21, 2018 01:19
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants