-
Notifications
You must be signed in to change notification settings - Fork 45
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
Add support for using goss to target Windows #39
Conversation
@fishnix would you be able to help? |
Hi @jsturtevant, thanks for the PR! I'll find some cycles to review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again for submitting this PR. I had a few comments/suggestions. I was going to ask you to change the variable name to target_os
until I noticed we've been entirely inconsistent with those properties 🤦 targetOs
works. Cheers!
packer-provisioner-goss.go
Outdated
@@ -33,6 +40,7 @@ type GossConfig struct { | |||
Password string | |||
SkipInstall bool | |||
Inspect bool | |||
TargetOs OsType `string:"targetOs"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this struct tag shouldn't be necessary
const ( | ||
Linux = OsType("Linux") | ||
Windows = OsType("Windows") | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I understand why you did this, but I'm inclined to simplify this and just use a string and get rid of the const. It will simplify the code below as well. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which logic? I think we still need all the checks in place as well as the ToLower
to ensure the url is written properly. I could replace the OsType
type and make it just a string?
arch := file[2] | ||
if p.isGossAlpha() { | ||
// includes .exe for windows | ||
arch = file[3] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the comment confused me at first (I thought maybe this was only to support windows with the.exe
- please strike it or clarify that the format of the alpha file is different.
if p.config.TargetOs != Linux && p.config.TargetOs != Windows { | ||
errs = packer.MultiErrorAppend(errs, | ||
fmt.Errorf("Os must be %s or %s", Linux, Windows)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Thanks for finding the time to review! I addressed the comments. If I pushed a github action for building and running the tests on PR would you be interested? |
output from a build:
|
@fishnix Thanks for the initial review. I addressed most comments would you take a look again? Thanks! |
@jsturtevant thanks for the fixes! 🙇 I'm sorry I haven't had a chance to review this. I will as soon as I can. Cheers! |
This provisioner is used to provide goss testing in the image-builder project. I recently added Windows support to image-builder and would like to run the goss test against it during provision in the same manner as the other images. Goss also now has alpha support for Windows.
I've updated the code here to support Windows and added a few tests.
Here is the output of it running against a windows VM. It is using a custom goss url while I wait for the fix in goss goss-org/goss#666