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

Proxy support for Initial install and shell provisioners #229

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

daxgames
Copy link
Contributor

@daxgames daxgames commented Feb 24, 2020

@arizvisa Merge my PRs in the order they were opened for best results. If merging all it should look like this when done.

They are all based on boxcutter/windows master branch.

The first 4 PRs do everything that was in #179

#229
#230
#232
#233

This is new based on issues encountered with New Packer 1.5.2

#234

Proxy support for Initial install and shell provisioners

  • Part of Prefix and proxy #179 so closing Prefix and proxy #179
  • Adds authenticated/unauthenticated http/https/no_proxy capability to _download.cmd for wget and powershell
    • See README.md for configuration documentation.
    • wget throws a lot of red onto the screen when using proxy so if proxy is configured powershell will be used. Note: this could be a configurable preferrence but I saw no need.
  • Moves powershell download to _download.cmd
  • documents use of floppy\_packer_config*.cmd
  • Add floppy/_packer_config_*.cmd to the .gitignore so users can customize without committing to this repo.

@daxgames
Copy link
Contributor Author

daxgames commented Feb 24, 2020

@arizvisa Proxy support added.

@daxgames
Copy link
Contributor Author

@arizvisa Still working on adding authenticated proxy.

@daxgames
Copy link
Contributor Author

@arizvisa This is ready to go. Please ask if you have any questions.

@daxgames daxgames changed the title Proxy Proxy support for Initial install and shell provisioners Feb 26, 2020
@daxgames daxgames mentioned this pull request Feb 26, 2020
@arizvisa
Copy link
Contributor

Thanks for doing this work. My bad on merging the other ones before reading this. :-/

I just relocated, so I don't have all my equipment setup to simulate this properly. It's likely I'll be able to deploy a quick proxy and build more than a single VM sometime next week, though. However, I'll see what I can do over the weekend so you aren't stuck waiting too long.

@arizvisa arizvisa self-assigned this Feb 28, 2020
@arizvisa arizvisa added the enhancement This will introduce or improve an already existing capability label Feb 28, 2020
@arizvisa arizvisa self-requested a review February 28, 2020 22:38
@daxgames
Copy link
Contributor Author

@arizvisa no worries. I oriiginally wrote a less capable version of the proxy support for this I think 3 years ago so I have been waiting for a while.

@arizvisa
Copy link
Contributor

As semi-discussed in #83, the addition of allowing users to customize the configuration during the floppy step (which includes the update to all of the templates) is different from the consolidation of the download methods that you did to implement proxy support.

Can you isolate the configuration (along with the relevant paragraph of documentation) and all of the modifications to the templates into its own PR?

@arizvisa arizvisa removed their request for review February 29, 2020 23:50
@daxgames
Copy link
Contributor Author

daxgames commented Mar 1, 2020

@arizvisa It was done because it is absolutely required to configure proxy support during floppy step. So it is indeed part of the proxy support PR.

Edit: Let me explain. You don't want someone editing a file that is a part of this repo to add optional configuration. They should be able to add a file that is ignored by git as optional configuration.

@arizvisa
Copy link
Contributor

arizvisa commented Mar 2, 2020

Is it though? What's wrong with the pre-existing methodology of editing floppy/_packer_config.cmd?

@daxgames
Copy link
Contributor Author

daxgames commented Mar 2, 2020

If you want to protect users and make it easy to use without making bad commits else its REALLY easy to mess up.

git add
git commit -m 'add proxy config'

I just don't see the features as a separate thing. In my opinion floppy/_packer_config*.cmd not being in the packer templates is an oversight that should have always been there.

Maybe DISABLE_BITS should be remove from floppy/_packer_config.cmd but I can see it becoming a default setting where proxy will never become a default setting. Which is why I put it in that file and suggest proxy config go in a user supplied ignored file.

@arizvisa
Copy link
Contributor

arizvisa commented Mar 2, 2020

Ok. No worries then. I'll do it for you.

@daxgames
Copy link
Contributor Author

daxgames commented Mar 2, 2020

@arizvisa You'll leave it in or take it out? I am not trying to be difficult, just curious.

I mean, I am just pleading my case and trying to convince you. You are the maintainer and have the ultimate say. I just disagree it is a mixed feature PR because the scripts have always had the ability to run multiple floppy/_packer_config*.cmd scripts.

I can make it a separate PR if that is what you really want. Just let me know.

arizvisa added a commit that referenced this pull request Mar 6, 2020
Added support for specifying additional floppy/_packer_config*.cmd files as part of PR #229.
@arizvisa
Copy link
Contributor

Can you re-base this? The differences should be a lot smaller as a result of PR #235.

@daxgames
Copy link
Contributor Author

@arizvisa Rebased

Copy link
Contributor

@arizvisa arizvisa left a comment

Choose a reason for hiding this comment

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

Good work on consolidating all the logic through _download.cmd. As mentioned in my review, I think if you're going to remove the else-case you might as well remove the entire conditional.

However, if you prefer to keep it, then at least let the user know that the download is actually being skipped due to the missing download script.

@@ -18,4 +18,3 @@ floppy/_packer_config_*.cmd
*.*.json
floppy/*.*.*
script/*.*.*

Copy link
Contributor

Choose a reason for hiding this comment

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

This stray new-line is unnecessary.

@@ -1,3 +1,4 @@

Copy link
Contributor

Choose a reason for hiding this comment

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

This newline is also unnecessary.

@@ -31,8 +31,8 @@ echo ==^> Copying "%found%" to "%filename%", skipping download.
copy /y "%found%" "%filename%" && goto exit0

:download

echo ==^> Downloading "%url%" to "%filename%"
REM IT IS JUST CLEANER WITH THIS WGET IS REALLY PICKY. POWERSHELL - NOT SO MUCH
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to justify this with a comment.. Can be removed as well.


set filename=%~2
set "filename=%~2"
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for properly quoting these. It's good style.

@@ -17,9 +17,6 @@ pushd "%BITVISE_DIR%"

if exist "%SystemRoot%\_download.cmd" (
call "%SystemRoot%\_download.cmd" "%BITVISE_URL%" "%BITVISE_PATH%"
) else (
Copy link
Contributor

Choose a reason for hiding this comment

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

These conditionals are used to detect if there's some kind of issue with the existence of the download script. If you're going to remove the "else" case, you might as well remove the whole conditional.

Ideally, though, you could emit an error and terminate the script if the download.cmd script is not found. However, either way is fine.

@@ -259,8 +253,9 @@ set SALT_URL=http://raw.githubusercontent.com/saltstack/salt-bootstrap/%SALT_REV
set SALT_PATH=%SALT_DIR%\bootstrap-salt.ps1
set SALT_DOWNLOAD=%SALT_DIR%\bootstrap-salt.download.ps1

echo ==^> Downloading %SALT_URL% to %SALT_DOWNLOAD%
powershell -Command "(New-Object System.Net.WebClient).DownloadFile('%SALT_URL%', '%SALT_DOWNLOAD%')" <NUL
if exist "%SystemRoot%\_download.cmd" (
Copy link
Contributor

Choose a reason for hiding this comment

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

(See prior comment)

@@ -301,7 +296,7 @@ if "%CM_VERSION%" == "latest" (
set SALT_PATH=%SALT_DIR%\Salt-Minion-Setup.exe

echo ==^> Downloading %SALT_URL% to %SALT_PATH%
powershell -Command "(New-Object System.Net.WebClient).DownloadFile('%SALT_URL%', '%SALT_PATH%')" <NUL
call "%SystemRoot%\_download.cmd" %SALT_URL% %SALT_PATH%
Copy link
Contributor

Choose a reason for hiding this comment

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

(See prior comment)

@@ -53,9 +53,6 @@ cd /d "%SEVENZIP_DIR%"

if exist "%SystemRoot%\_download.cmd" (
call "%SystemRoot%\_download.cmd" "%SEVENZIP_URL%" "%SEVENZIP_PATH%"
) else (
Copy link
Contributor

Choose a reason for hiding this comment

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

(See comment for floppy/bitvisessh.bat)

@@ -131,9 +128,6 @@ pushd "%ULTRADEFRAG_DIR%"

if exist "%SystemRoot%\_download.cmd" (
call "%SystemRoot%\_download.cmd" "%ULTRADEFRAG_URL%" "%ULTRADEFRAG_PATH%"
) else (
Copy link
Contributor

Choose a reason for hiding this comment

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

(See comment for floppy/bitvisessh.bat)

@@ -15,9 +15,6 @@ pushd "%VAGRANT_DIR%"

if exist "%SystemRoot%\_download.cmd" (
call "%SystemRoot%\_download.cmd" "%VAGRANT_PUB_URL%" "%VAGRANT_PATH%"
) else (
Copy link
Contributor

Choose a reason for hiding this comment

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

(See comment for floppy/bitvisessh.bat)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This will introduce or improve an already existing capability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants