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

Add new options to Command resource #434

Merged
merged 5 commits into from May 12, 2019

Conversation

Projects
None yet
2 participants
@sshipway
Copy link
Contributor

commented Apr 5, 2019

This pull request does the following:

  • Add 'exec' attribute to Command resource defaulting to hash key
  • Add 'skip' attribute to most resources to allow them to be easily disabled when running a deepmerge of the yaml (e.g. in puppet config management tool)
  • Documentation changes for the above
  • Update of acceptance tests to reflect new exec attribute defaults
@sshipway

This comment has been minimized.

Copy link
Contributor Author

commented Apr 7, 2019

Looks like the travis build check is failing because it is using an outdated version of golang that expects no arguments to getMounts. Part of this patch adds the (nil) parameter to the getMounts() call for compatibility.

@aelsabbahy

This comment has been minimized.

Copy link
Owner

commented Apr 23, 2019

@sshipway the travis check uses go dep to install specific versions of the dependences. The correct use is GetMounts() for the version that's locked in the https://github.com/aelsabbahy/goss/blob/master/Gopkg.lock

Running make deps locally may resolve your local build issue.

@sshipway

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

Looks like I need to install 'godep' and set up a whole lot of softlinks to make it work. I'll revert the GetMounts change in my branch and see if it then passes travis.

@sshipway

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

@aelsabbahy - It now works using the older docker/docker library and I have godep doing its thing, and it's passed Travis. Go seems to be rather fond of github. Thanks for the pointers, hope these improvements can make it into a new release.

Add optional exec attribute to command definitions
Allow skip attribute in command definitions

Documentation changes for new command exec attribute, and skip attribute

Update CI tests to match changes to the command exec attribute

@sshipway sshipway force-pushed the sshipway:master branch from a3b8f0f to 60223a2 Apr 26, 2019

@sshipway

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

Squashed all commits to a single one for clarity

@aelsabbahy

This comment has been minimized.

Copy link
Owner

commented Apr 26, 2019

A few things:

  1. Goss add command shouldn't add the exec element by default. It should be a manual change that a user can make.
  2. Can you add some tests showcasing the skip flag, maybe a skipped test for a file or a service in goss-shared.yaml
  3. Related to 2, when I compiled a local version of this and ran it with a skipped test (file) the code panicked.
@sshipway

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

  1. Removed the default exec=command setting when adding
  2. Added these for command, file and service; as well as a test of the exec attribute
  3. When using skip attribute to skip the first test in a series, shouldSkip() on subsequent tests would then fail as the Found array would be empty. Added some checks on array size to the function to prevent this.

Note that there seems to be an issue on my host with running test.sh where the containers need to be started with --privileged for the systemd to start the httpd. Not sure why this should be, I'm looking into it.

aelsabbahy and others added some commits Apr 23, 2019

Disable dnstest (#440)
* Disable dnstest.io tests, seems to be down

* Update md5 for latest docker images
Add tests for command and skip
Fix problem with skipping first test
Fix where commands add exec attribute automatically

@sshipway sshipway force-pushed the sshipway:master branch from 206948a to 0c25998 Apr 27, 2019

@sshipway

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2019

All done. There was a conflict with the other DNS test changes when I tried to squash, so I merged and resolved at my end. Hope this is OK.
This has been an interesting first experience with go and travis...

@aelsabbahy

This comment has been minimized.

Copy link
Owner

commented May 2, 2019

The PR looks great so far, did some initial quick test and it appears to be working.

Couple of things:

  1. Can you run all the changed files through either gofmt or goimports
  2. I'm wondering if it's clearer to add Found: []string{"false"}, to https://github.com/aelsabbahy/goss/blob/master/resource/validate.go#L58 rather than changing shouldskip the conditions for true/false currently seem a little confusing to me

Thanks for the awesome contribution.

@sshipway

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

  1. I've run gofmt on it (another new utility for me); it apparently loves tabs and vertical space...
  2. I was trying to fit in with the existing skip functionality, so as to minimise the required changes; TBH I'm not certain how the Found change would affect behaviour. I felt it clearer and safer to extend shouldskip to remove the assumption that there are preexisting test results instead, as this make the code more robust. Personally I think this way clearer; however, your project, your decision.

This is the first time I've used Travis, and I'm a total convert. For open-source projects with multiple PR coming in, and an Agile workflow, it will save hours of work. If gofmt is a non-negotiable for PR acceptance, maybe add a gofmt check to the Travis as well?

@aelsabbahy

This comment has been minimized.

Copy link
Owner

commented May 12, 2019

Looks great, thanks.

Opened an issue to #445 to add gofmt check as part of CI, thanks!

@aelsabbahy aelsabbahy merged commit e570116 into aelsabbahy:master May 12, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.