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

Swap windows jobs to docker (as much as possible) #6603

Merged
merged 5 commits into from Jun 6, 2019

Conversation

@scotthain
Copy link
Contributor

commented May 30, 2019

Signed-off-by: Scott Hain shain@chef.io

There are a few jobs that still fail.

@chef-expeditor

This comment has been minimized.

Copy link

commented May 30, 2019

Hello scotthain! Thanks for the pull request!

Here is what will happen next:

  1. Your PR will be reviewed by the maintainers.
  2. If everything looks good, one of them will approve it, and your PR will be merged.

Thank you for contributing!

@mwrock
Copy link
Contributor

left a comment

this looks good to me. just have 1 thing that needs to change so this can still pass locally

support/ci/shared.ps1 Outdated Show resolved Hide resolved

@mwrock mwrock force-pushed the shain/docker2 branch from 19faa7d to af79875 Jun 3, 2019

@scotthain scotthain requested review from chefsalim and eeyun as code owners Jun 4, 2019

scotthain and others added some commits May 28, 2019

Swap windows jobs to docker
Signed-off-by: Scott Hain <shain@chef.io>
Selectively disable failing docker tests"
Signed-off-by: Scott Hain <shain@chef.io>
only use choco artifactory source on buildkite
Signed-off-by: mwrock <matt@mattwrock.com>

@mwrock mwrock force-pushed the shain/docker2 branch from a9e32a9 to a7c7708 Jun 4, 2019

@mwrock

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

I've gone and made a few changes to allow tests to succeed in a windows container environment. Here is a summary:

  • Remove use of canonicalize in fs test code. Its a known issue that this method is not friendly to certain Windows file systems including docker mounts.
  • Interactive promote test should not try to use cat as a UI editor. I do not know how this could have ever worked and fails for me locally outside of docker.
  • Use copy and remove_file instead of rename when generating the protocols. Rename will not work over a mount.
  • set the default rustup host to x86_64-pc-windows-msvc. It was x86_64-pc-windows-gnu which was breaking some butterfly test compilations that was trying to find gcc.exe.
  • Make sure the studio tests have needed prereqs (habitat and nuget provider)

@mwrock mwrock self-assigned this Jun 5, 2019

@scotthain

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

This looks great @mwrock - I think you need to reapprove it and then I can merge it if you're ok with it. Thanks for picking up the ball on this!!

@mwrock mwrock force-pushed the shain/docker2 branch 2 times, most recently from 6d666a6 to b141146 Jun 5, 2019

@mwrock

mwrock approved these changes Jun 5, 2019

.expeditor/verify.pipeline.yml Show resolved Hide resolved
@@ -822,8 +821,8 @@ mod test_find_command {
fn setup_path() {

This comment has been minimized.

Copy link
@baumanj

baumanj Jun 5, 2019

Member

This function modifies global state (via env::set_var), but will be run by multiple tests concurrently. We should use something like locked_env_var to make this thread-safe or restructure the tests with dependency injection to not depend on global state. I think the latter would be preferable since it doesn't limit the concurrency potential for the tests.

This comment has been minimized.

Copy link
@mwrock

mwrock Jun 5, 2019

Contributor

locked_env_var is defined in common so we would have to move it to core and adjust all callers which feels like it belongs in a separate PR.

This comment has been minimized.

Copy link
@baumanj

baumanj Jun 5, 2019

Member

Yeah, it's overdue to be moved into core (probably common/core should be merged entirely), but I think it's worth doing now since these tests are prone to non-deterministic failures without it. A separate PR is good, but it could merge before this one so we can base the fix on it.

How about taking the dependency-injection approach? That's the preferable solution in my view and is appropriate to this PR scope.

This comment has been minimized.

Copy link
@scotthain

scotthain Jun 5, 2019

Author Contributor

Including that in here feels to me kinda like scope creep for this PR but I'll defer to you both on it!

This comment has been minimized.

Copy link
@baumanj

baumanj Jun 5, 2019

Member

I agree that moving locked_env_var deserves its own PR if we go that way. If we use dependency injection instead, we can make those changes in this PR and leave moving locked_env_var for later, but either way I think we should not leave this in a subtly broken state which may crop up some unknown time in the future.

This comment has been minimized.

Copy link
@mwrock

mwrock Jun 5, 2019

Contributor

In this case, I think even if multiple tests concurrently change this, its harmless. worse that will happen id the fixtures will get added to the path multiple times.

This comment has been minimized.

Copy link
@baumanj

baumanj Jun 6, 2019

Member

I'm not so sure we can make that strong an assumption about the way env::set_var works. The documentation says "concurrent access to environment variables is safe in Rust", but that doesn't mean that it will do what you expect. I'm not sure we're guaranteed one thread accessing this in the midst of another thread setting it couldn't result in an empty result.

In any case, it's not a huge deal, but it's an error-prone pattern to rely on global state like this, so when we can switch to something more robust (such as injecting global dependencies as arguments) without a huge hassle, I think we'd be well served to do so.

This comment has been minimized.

Copy link
@mwrock

mwrock Jun 6, 2019

Contributor

Looking at this I still think it is out of scope. These tests while not ideal has been in place for 3 years without raising stability issues. I'd just rather not go into refactoring non-test code unrelated to the intent of this PR and get it merged.

This comment has been minimized.

Copy link
@baumanj

baumanj Jun 6, 2019

Member

In test code, I'm equally or more worried about false positives, but you're right that we can probably let this one go. The value of getting this merged is quite high.

components/hab/src/command/bldr/job/promote.rs Outdated Show resolved Hide resolved
components/sup-protocol/build.rs Outdated Show resolved Hide resolved
@scotthain

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

I'm going to defer to @mwrock to answer questions on the rust changes.

@mwrock mwrock force-pushed the shain/docker2 branch 2 times, most recently from 4c0401e to 14600f0 Jun 5, 2019

make clippy and all tests windows docker friendly
Signed-off-by: mwrock <matt@mattwrock.com>

@mwrock mwrock force-pushed the shain/docker2 branch from 14600f0 to 96bc4c4 Jun 5, 2019

@scotthain scotthain force-pushed the shain/docker2 branch from 6dca48d to ca4b598 Jun 6, 2019

Don't use artifactory mirror
Signed-off-by: Scott Hain <shain@chef.io>

@scotthain scotthain force-pushed the shain/docker2 branch from 160e929 to 48f60f5 Jun 6, 2019

@baumanj

baumanj approved these changes Jun 6, 2019

@mwrock mwrock merged commit 282ed3c into master Jun 6, 2019

5 checks passed

DCO This commit has a DCO Signed-off-by
Details
buildkite/habitat-sh-habitat-master-verify Build #2194 passed (54 minutes, 29 seconds)
Details
buildkite/habitat-sh-habitat-master-website Build #2426 passed (32 seconds)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
expeditor/config-validation Validated your Expeditor config file
Details

@mwrock mwrock deleted the shain/docker2 branch Jun 6, 2019

chef-ci added a commit that referenced this pull request Jun 6, 2019

Update CHANGELOG.md with details from pull request #6603
Obvious fix; these changes are the result of automation not creative thinking.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.