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

scripts/checks: adjust to ensure go 1.4.2 is in use #144

Merged
merged 2 commits into from
Sep 18, 2015
Merged

Conversation

erikh
Copy link
Contributor

@erikh erikh commented Sep 2, 2015

Signed-off-by: Erik Hollensbe [email protected]

@mapuri
Copy link
Contributor

mapuri commented Sep 2, 2015

@erikh

Thinking more this change in itself might not actually work as the checks get triggered as part of build which happens inside the vm, so I think they will always succeed. May be we will need to add checks as part of systemtest target?
Other than that the change looks good.

++++++
And now some ranting ;)

I have a slight reservation with this stricter check that it might come as restricting for users running a slightly old or new version of go1.4 on host. We had similar check before and had to relax it as part of #30. Also I am not sure if current systemtests build fine for mac users, do you know? I remember @shaleman mentioned that systemtest still don't build on his mac but not sure if he is still hitting it.

Thinking about alternate approaches. I experimented with modifying the system-test-singlehost target as follows, which works on my server. But it will not work for mac users though. However, just wanted to share in case it seems better than a stricter version check and if we can't run systemtests mac for other reasons:

system-test-singlehost: clean run-build
     godep go test -v --timeout 30m -run "sanity" \
                      github.com/contiv/netplugin/systemtests/singlehost

@erikh
Copy link
Contributor Author

erikh commented Sep 2, 2015

It works for the host build, which IIRC was the problem. The restriction lives to keep people from building bad copies.

The systemtests do not work on mac -- they need the netlink library.

@mapuri
Copy link
Contributor

mapuri commented Sep 2, 2015

The restriction lives to keep people from building bad copies.

sgtm

we still need to add checks target as part of systemtest, right?

@erikh
Copy link
Contributor Author

erikh commented Sep 2, 2015

Yeah, probably a good idea. Let me amend this, one minute.

@erikh
Copy link
Contributor Author

erikh commented Sep 2, 2015

this should be ready after tests pass.

@erikh
Copy link
Contributor Author

erikh commented Sep 10, 2015

Anything left to do here?

@mapuri
Copy link
Contributor

mapuri commented Sep 10, 2015

changes look good, it needs a rebase though.

@erikh
Copy link
Contributor Author

erikh commented Sep 16, 2015

once CI passes, let's merge this

erikh pushed a commit that referenced this pull request Sep 18, 2015
scripts/checks: adjust to ensure go 1.4.2 is in use
@erikh erikh merged commit da9d00d into master Sep 18, 2015
@erikh erikh deleted the go1.4.2-check branch September 18, 2015 13:35
@erikh
Copy link
Contributor Author

erikh commented Sep 18, 2015

@mapuri I merged this so we could move forward with @shaleman's changes. I'm going to do the same for the systemtests stuff today.

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.

2 participants