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

Multi-OS: Provide binaries #585

Merged
merged 71 commits into from
Jun 16, 2020

Conversation

petemounce
Copy link
Collaborator

@petemounce petemounce commented May 10, 2020

Fixes #551, #405, #406, #403, #448, #385, #380, #298 (or supercedes them)

  • See Travis make binaries and pass
    • linux
      • binary
      • lint + short tests
      • integration tests
    • macOS
      • binary
      • lint + short tests
    • Windows
      • binary
      • lint + short tests
  • Write OS-support matrix doc per Discussion: Multi-OS support #551
  • Link doc from readme
Checklist
  • make test-all (UNIX) passes. CI will also test this
  • unit and/or integration tests are included (if applicable)
  • documentation is changed or added (if applicable)

Description of change

Adds binaries for macOS and Windows, plus feature-parity matrix doc / test-plan.

.travis.yml Outdated
Comment on lines 24 to 35
install:
- go get -u golang.org/x/lint/golint
- curl -L https://codeclimate.com/downloads/test-reporter/test-reporter-latest-linux-amd64 > ./cc-test-reporter
- chmod +x ./cc-test-reporter
- ./ci/install.sh "${TRAVIS_OS_NAME}"

before_script:
- ./cc-test-reporter before-build
- ./ci/before-build.sh "${TRAVIS_OS_NAME}"

script:
- make all
- ./ci/build.sh "${TRAVIS_OS_NAME}"

after_script:
- ./cc-test-reporter after-build --exit-code $TRAVIS_TEST_RESULT -d
- ./ci/after-build.sh "${TRAVIS_OS_NAME}"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've factored these commands into scripts where it's cleaner to account for the cross-platform differences than within the CI configuration.


test-all: fmt lint vet test test-int-all
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I introduced test-short-all to represent all the short tests like lints and unit-tests, which can run on each platform.

The integration tests can't run on non-linux unless we come up with a different approach, but I definitely didn't want to include that scope here.

Copy link
Member

Choose a reason for hiding this comment

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

Honestly, I give up on trying to have a proper integration test for Windows/OSX

Solid unit tests + a small smoke test might be sufficient.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, 👍. If we think it becomes worthwhile to do more than unit-test, I reckon it'll be reasonably straightforward to achieve via TestMain and blackbox'ing against some golden files.

Copy link
Member

@aelsabbahy aelsabbahy May 17, 2020

Choose a reason for hiding this comment

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

Curious on your Test mail suggestion. Whenever you have a chance (no rush). Can you link me a psudocode gist example of what you mean.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

https://github.com/aelsabbahy/goss/pull/407/files#diff-2e387891f2d94197d8fc70573226de67R17-R34 - the job of the TestMain is to make sure that the binary is present; in this example it's done via go build, but it doesn't matter what the implementation is.

https://github.com/aelsabbahy/goss/pull/407/files#diff-2e387891f2d94197d8fc70573226de67R1 - use a build-tag so that these tests only run via go test -tags integration - this would allow the tests to only be run after the build automation has had a chance to build the release binaries, at which point TestMain could just choose which one to use based on runtime.GOOS/runtime.GOARCH.

https://github.com/aelsabbahy/goss/pull/407/files#diff-2e387891f2d94197d8fc70573226de67R13 - rendon/testcli is a package that looked the most similar to a setup I'm familiar with from another language's ecosystem. It makes a small wrapper around os.exec and then allows one to assert against stdout/stderr.

https://github.com/aelsabbahy/goss/pull/407/files#diff-2e387891f2d94197d8fc70573226de67R36-R63 - table-driven tests that run the binary and compare the output to some golden files, with the option to update those.


install: release/goss-linux-amd64
$(info INFO: Starting build $@)
cp release/$(cmd)-linux-amd64 $(GOPATH)/bin/goss

test:
$(info INFO: Starting build $@)
{ \
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Similar to inside travis, factored these into scripts where it becomes easier to deal with the cross-platform differences.


| Feature | `linux` implementation + unit-tests | `linux` integration-tests | `macOS` implementation + unit-tests | `macOS` integration-tests | `Windows` implementation + unit-tests | `Windows` integration-tests |
|:---------------|:--------|:-----------------:|:----------------:|:-----------------:|:----------------:|:-------------------:|:------------------:|
| Assertion: `addr` | √ | √ | | | | |
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can format this via a tool, but honestly I think that will just make it more fiddly to edit; tables in markdown are fiddly, so I've done the bare minimum.

I don't think this is complete, but it's illustrative. What do you think?

Copy link
Member

@aelsabbahy aelsabbahy May 10, 2020

Choose a reason for hiding this comment

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

This doesn't render correctly for me. Maybe a typo somewhere?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm.. does Linux really have unit tests?

Maybe the columns need to be:

Feature, linux, windows, osx

And symbols for fully support, partial support, and no support.

Maybe for partial support, explain the subset of attributes that don't work.

Thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Columns sounds good to me. It works conceptually, but I think will be hard to deal with inside the table formatting. I'll make headings under the table and page links from the cells where more detail is needed. What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How's this? It's now basically a full test-plan, ready to be filled out. I don't intend to fill that out inside this PR more than what I've done to illustrate how to do it, though; sound ok?

@petemounce
Copy link
Collaborator Author

Separately from this PR I intend to:

make some changes into the integrations test script(s) as mentioned within #586, to separate the setup from the test runs to decouple and make them a bit easier to debug.
get some feedback from you on the TestMain+golden files golang integration test approach I mentioned in a thread above.

Can you create a separate issue to discuss this further. Want to make sure we're coordinated before doing the actual work.

I've expanded the description inside #586 since that existed already. If you prefer, I can transfer the bulk of that to an issue? I was partly responding to your comment in this PR, partly to #586 (comment).

Copy link
Member

@aelsabbahy aelsabbahy left a comment

Choose a reason for hiding this comment

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

Couple of comments.. and a question.

Question, why do the travis-ci for windows and osx differ in printing alpha warning?


#### Windows `add`

```powershell
Copy link
Member

Choose a reason for hiding this comment

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

curious.. why does travis-CI not show this message?

Copy link
Collaborator Author

@petemounce petemounce Jun 10, 2020

Choose a reason for hiding this comment

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

The goss log. output goes to stderr and (apparently, I didn't know this until just now) travis doesn't capture stderr.

You can try this out locally:

TRAVIS_OS_NAME="windows "ci/build.sh "windows" >stdout.txt

and you will see only the stderr output since stdout will now appear only within stdout.txt.

Here is the output I get:

Pete@monsterbox ~/src/improbable/goss (multi-os-binaries)
λ TRAVIS_OS_NAME=windows ci/build.sh windows > stdout.txt
/usr/bin/sh: golint: command not found
2020/06/10 10:16:42 WARNING: goss for this platform ("Windows") is alpha-quality, work-in-progress, and not yet exercised within continuous integration.

You should not expect everything to work. Treat linux as the canonical behaviour to expect.

Please see https://github.com/aelsabbahy/goss/tree/master/docs/platform-feature-parity.md to set your expectations and see progress.
Please file issues via https://github.com/aelsabbahy/goss/issues/new/choose
Pull requests and bug reports very welcome.
2020/06/10 10:16:42 WARNING: goss for this platform ("Windows") is alpha-quality, work-in-progress, and not yet exercised within continuous integration.

You should not expect everything to work. Treat linux as the canonical behaviour to expect.

Please see https://github.com/aelsabbahy/goss/tree/master/docs/platform-feature-parity.md to set your expectations and see progress.
Please file issues via https://github.com/aelsabbahy/goss/issues/new/choose
Pull requests and bug reports very welcome.
2020/06/10 10:16:42 WARNING: goss for this platform ("Windows") is alpha-quality, work-in-progress, and not yet exercised within continuous integration.

You should not expect everything to work. Treat linux as the canonical behaviour to expect.

Please see https://github.com/aelsabbahy/goss/tree/master/docs/platform-feature-parity.md to set your expectations and see progress.
Please file issues via https://github.com/aelsabbahy/goss/issues/new/choose
Pull requests and bug reports very welcome.
2020/06/10 10:16:42 WARNING: goss for this platform ("Windows") is alpha-quality, work-in-progress, and not yet exercised within continuous integration.

You should not expect everything to work. Treat linux as the canonical behaviour to expect.

Please see https://github.com/aelsabbahy/goss/tree/master/docs/platform-feature-parity.md to set your expectations and see progress.
Please file issues via https://github.com/aelsabbahy/goss/issues/new/choose
Pull requests and bug reports very welcome.
2020/06/10 10:16:42 WARNING: goss for this platform ("Windows") is alpha-quality, work-in-progress, and not yet exercised within continuous integration.

You should not expect everything to work. Treat linux as the canonical behaviour to expect.

Please see https://github.com/aelsabbahy/goss/tree/master/docs/platform-feature-parity.md to set your expectations and see progress.
Please file issues via https://github.com/aelsabbahy/goss/issues/new/choose
Pull requests and bug reports very welcome.
2020/06/10 10:16:42 WARNING: goss for this platform ("Windows") is alpha-quality, work-in-progress, and not yet exercised within continuous integration.

You should not expect everything to work. Treat linux as the canonical behaviour to expect.

Please see https://github.com/aelsabbahy/goss/tree/master/docs/platform-feature-parity.md to set your expectations and see progress.
Please file issues via https://github.com/aelsabbahy/goss/issues/new/choose
Pull requests and bug reports very welcome.
2020/06/10 10:16:43 WARNING: goss for this platform ("Windows") is alpha-quality, work-in-progress, and not yet exercised within continuous integration.

You should not expect everything to work. Treat linux as the canonical behaviour to expect.

Please see https://github.com/aelsabbahy/goss/tree/master/docs/platform-feature-parity.md to set your expectations and see progress.
Please file issues via https://github.com/aelsabbahy/goss/issues/new/choose
Pull requests and bug reports very welcome.
2020/06/10 10:16:43 WARNING: goss for this platform ("Windows") is alpha-quality, work-in-progress, and not yet exercised within continuous integration.

You should not expect everything to work. Treat linux as the canonical behaviour to expect.

Please see https://github.com/aelsabbahy/goss/tree/master/docs/platform-feature-parity.md to set your expectations and see progress.
Please file issues via https://github.com/aelsabbahy/goss/issues/new/choose
Pull requests and bug reports very welcome.
2020/06/10 10:16:43 WARNING: goss for this platform ("Windows") is alpha-quality, work-in-progress, and not yet exercised within continuous integration.

You should not expect everything to work. Treat linux as the canonical behaviour to expect.

Please see https://github.com/aelsabbahy/goss/tree/master/docs/platform-feature-parity.md to set your expectations and see progress.
Please file issues via https://github.com/aelsabbahy/goss/issues/new/choose
Pull requests and bug reports very welcome.
2020/06/10 10:16:43 WARNING: goss for this platform ("Windows") is alpha-quality, work-in-progress, and not yet exercised within continuous integration.

You should not expect everything to work. Treat linux as the canonical behaviour to expect.

Please see https://github.com/aelsabbahy/goss/tree/master/docs/platform-feature-parity.md to set your expectations and see progress.
Please file issues via https://github.com/aelsabbahy/goss/issues/new/choose
Pull requests and bug reports very welcome.
2020/06/10 10:16:43 WARNING: goss for this platform ("Windows") is alpha-quality, work-in-progress, and not yet exercised within continuous integration.

You should not expect everything to work. Treat linux as the canonical behaviour to expect.

Please see https://github.com/aelsabbahy/goss/tree/master/docs/platform-feature-parity.md to set your expectations and see progress.
Please file issues via https://github.com/aelsabbahy/goss/issues/new/choose
Pull requests and bug reports very welcome.
2020/06/10 10:16:43 WARNING: goss for this platform ("Windows") is alpha-quality, work-in-progress, and not yet exercised within continuous integration.

You should not expect everything to work. Treat linux as the canonical behaviour to expect.

Please see https://github.com/aelsabbahy/goss/tree/master/docs/platform-feature-parity.md to set your expectations and see progress.
Please file issues via https://github.com/aelsabbahy/goss/issues/new/choose
Pull requests and bug reports very welcome.
2020/06/10 10:16:43 WARNING: goss for this platform ("Windows") is alpha-quality, work-in-progress, and not yet exercised within continuous integration.

You should not expect everything to work. Treat linux as the canonical behaviour to expect.

Please see https://github.com/aelsabbahy/goss/tree/master/docs/platform-feature-parity.md to set your expectations and see progress.
Please file issues via https://github.com/aelsabbahy/goss/issues/new/choose
Pull requests and bug reports very welcome.
2020/06/10 10:16:43 WARNING: goss for this platform ("Windows") is alpha-quality, work-in-progress, and not yet exercised within continuous integration.

You should not expect everything to work. Treat linux as the canonical behaviour to expect.

Please see https://github.com/aelsabbahy/goss/tree/master/docs/platform-feature-parity.md to set your expectations and see progress.
Please file issues via https://github.com/aelsabbahy/goss/issues/new/choose
Pull requests and bug reports very welcome.
2020/06/10 10:16:43 WARNING: goss for this platform ("Windows") is alpha-quality, work-in-progress, and not yet exercised within continuous integration.

You should not expect everything to work. Treat linux as the canonical behaviour to expect.

Please see https://github.com/aelsabbahy/goss/tree/master/docs/platform-feature-parity.md to set your expectations and see progress.
Please file issues via https://github.com/aelsabbahy/goss/issues/new/choose
Pull requests and bug reports very welcome.
2020/06/10 10:16:43 WARNING: goss for this platform ("Windows") is alpha-quality, work-in-progress, and not yet exercised within continuous integration.

You should not expect everything to work. Treat linux as the canonical behaviour to expect.

Please see https://github.com/aelsabbahy/goss/tree/master/docs/platform-feature-parity.md to set your expectations and see progress.
Please file issues via https://github.com/aelsabbahy/goss/issues/new/choose
Pull requests and bug reports very welcome.
2020/06/10 10:16:43 WARNING: goss for this platform ("Windows") is alpha-quality, work-in-progress, and not yet exercised within continuous integration.

You should not expect everything to work. Treat linux as the canonical behaviour to expect.

Please see https://github.com/aelsabbahy/goss/tree/master/docs/platform-feature-parity.md to set your expectations and see progress.
Please file issues via https://github.com/aelsabbahy/goss/issues/new/choose
Pull requests and bug reports very welcome.
2020/06/10 10:16:43 WARNING: goss for this platform ("Windows") is alpha-quality, work-in-progress, and not yet exercised within continuous integration.

You should not expect everything to work. Treat linux as the canonical behaviour to expect.

Please see https://github.com/aelsabbahy/goss/tree/master/docs/platform-feature-parity.md to set your expectations and see progress.
Please file issues via https://github.com/aelsabbahy/goss/issues/new/choose
Pull requests and bug reports very welcome.

}

func (f *DefFile) Owner() (string, error) {
return getUserForUid(1000)
Copy link
Member

Choose a reason for hiding this comment

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

no data is better than wrong data? What does the hardcoded 1000 do here?

Copy link
Member

Choose a reason for hiding this comment

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

I'd rather return a blank or not-implemented.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in 143e895

@aelsabbahy
Copy link
Member

My plan is to merge this next week and released with a couple of other PRs. Didn't get any feedback from those tagged.. so.. might as well just get this wrapped up and released.

@aelsabbahy
Copy link
Member

@petemounce This is pretty much queued for the next release, just waiting on the two points above.

@petemounce
Copy link
Collaborator Author

@petemounce This is pretty much queued for the next release, just waiting on the two points above.

Awesome.

@aelsabbahy
Copy link
Member

aelsabbahy commented Jun 15, 2020

Pulled the code down locally to give it a quick sanity check before merging.

$ make

INFO: Starting build fmt
./ci/go-fmt.sh
./ci/go-fmt.sh: line 4: TRAVIS_OS_NAME: No value from TRAVIS_OS_NAME. This is meant to be run in Travis CI, see also https://docs.travis-ci.com/user/environment-variables/#convenience-variables
make: *** [Makefile:32: fmt] Error 1

Seems this PR is a little too aggressive in what it requires travis_ci for. I should be able to continue to run make or make fmt locally.

@petemounce
Copy link
Collaborator Author

Definitely. I'll fix that now.

@petemounce
Copy link
Collaborator Author

@aelsabbahy I don't understand why currently, but one linux http integration test is currently failing - goss-shared.yaml:L191-198.

When I curl that address I get a 200 OK in a tiny amount of time (I've taken out the schannel parts as noise):

λ  curl.exe --verbose https://httpbin.org/headers?host -H "host: example.com"
*   Trying 34.235.192.52...
* TCP_NODELAY set
* Connected to httpbin.org (34.235.192.52) port 443 (#0)

> GET /headers?host HTTP/1.1
> host: example.com
> User-Agent: curl/7.55.1
> Accept: */*
>

< HTTP/1.1 200 OK
< Date: Mon, 15 Jun 2020 21:35:24 GMT
< Content-Type: application/json
< Content-Length: 173
< Connection: keep-alive
< Server: gunicorn/19.9.0
< Access-Control-Allow-Origin: *
< Access-Control-Allow-Credentials: true
<
{
  "headers": {
    "Accept": "*/*",
    "Host": "example.com",
    "User-Agent": "curl/7.55.1",
    "X-Amzn-Trace-Id": "Root=1-5ee7e99c-ddd05894ae051ba4deffd380"
  }
}
* Connection #0 to host httpbin.org left intact

Whereas the integration test goss output is

HTTP: https://httpbin.org/headers?host: status:
Expected
    <int>: 404
to equal
    <int>: 200
HTTP: https://httpbin.org/headers?host: Headers: patterns not found: [Content-Type: application/json]
HTTP: https://httpbin.org/headers?host: Body: patterns not found: ["Host": "example.com"]

I'm somewhat at a loss - that looks like it should be passing to me?

@aelsabbahy
Copy link
Member

Ugh.. I'll look at this tomorrow, flakey tests annoy me.

Really need to switch to a completely controlled test environment and just drop the external dependencies.

This could be fun for unit/integration tests on the to level.
https://github.com/testcontainers/testcontainers-go

@petemounce
Copy link
Collaborator Author

Oh, also - make test-short-all runs and passes locally on my Windows within git-bash.

Pete@monsterbox MINGW64 ~/src/improbable/goss (multi-os-binaries)
$ make test-short-all
INFO: Starting build fmt
./ci/go-fmt.sh
/c/Go/bin/gofmt
Skipping go-fmt on Windows because line-endings cause every file to need formatting.
Linux is treated as authoritative.
Exiting 0...
INFO: Starting build lint
golint . ./cmd/... ./matchers/... ./outputs/... ./resource/... ./system/... ./util/... || true
/usr/bin/sh: golint: command not found
INFO: Starting build vet
go vet . ./cmd/... ./matchers/... ./outputs/... ./resource/... ./system/... ./util/... || true
INFO: Starting build test
./ci/go-test.sh . ./cmd/... ./matchers/... ./outputs/... ./resource/... ./system/... ./util/...
/c/Go/bin/go
ok      github.com/aelsabbahy/goss      0.279s  coverage: 45.9% of statements

The integration tests fail on Windows because git-bash doesn't come with /sbin/init - but that's ok, I think, until/unless those are containerised for Windows (I don't think there's much value in supporting Windows-host/Linux-container tests currently because I think most people (happy to be wrong!) are likely to lean on CI for that suite of tests).

@petemounce
Copy link
Collaborator Author

Ugh.. I'll look at this tomorrow, flakey tests annoy me.

Really need to switch to a completely controlled test environment and just drop the external dependencies.

This could be fun for unit/integration tests on the to level.
https://github.com/testcontainers/testcontainers-go

That looks nice.

re: environment - we could lean on a docker-compose file to stand up local DNS/http/etc endpoints to aim at, at some point? I hate flaky tests too.

@aelsabbahy
Copy link
Member

re: environment - we could lean on a docker-compose file to stand up local DNS/http/etc endpoints to aim at, at some point? I hate flaky tests too.

Agree. I go back and forth in my head if I want to be more unit test heavy or not.

By the way, restarting the build got it green.

@petemounce
Copy link
Collaborator Author

re: environment - we could lean on a docker-compose file to stand up local DNS/http/etc endpoints to aim at, at some point? I hate flaky tests too.

Agree. I go back and forth in my head if I want to be more unit test heavy or not.

I think flakes are insidiously bad, so I'm inclined to own remote dependencies where possible.

By the way, restarting the build got it green.

🤕

@aelsabbahy
Copy link
Member

I just noticed that the build dependency on GO_FILES is missing, when I switch branches and do a make install I got the old version of goss.

This isn't something I'll hold the PR over.. but curious if it was moved intentionally or just missed with the Makefile refactor.

If not intentional, I can create a quick change adding it back in.

@aelsabbahy aelsabbahy merged commit eb26f55 into goss-org:master Jun 16, 2020
@aelsabbahy
Copy link
Member

Thank you so much for this. A lot of time and effort went into this work and it is greatly appreciated!

@petemounce
Copy link
Collaborator Author

My pleasure!

@petemounce
Copy link
Collaborator Author

I just noticed that the build dependency on GO_FILES is missing, when I switch branches and do a make install I got the old version of goss.

This isn't something I'll hold the PR over.. but curious if it was moved intentionally or just missed with the Makefile refactor.

If not intentional, I can create a quick change adding it back in.

I think that was inadvertent. Makefiles are not a core skill of mine - I thought it was unused but missed that it was being used as a dependency to invalidate builds to make rebuilds happen.

In one of the earlier commits I found a way to do it cross platform with go ls-files I think - the find didn't work on all 3 platforms.

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

Successfully merging this pull request may close these issues.

Discussion: Multi-OS support
2 participants