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

openshift: 1.5.0 -> 3.6.0 #28345

Merged
merged 3 commits into from
Sep 14, 2017
Merged

openshift: 1.5.0 -> 3.6.0 #28345

merged 3 commits into from
Sep 14, 2017

Conversation

moretea
Copy link
Contributor

@moretea moretea commented Aug 17, 2017

Unfortunately, oc cluster up still does not work after this update (but neither did it work on 1.5.0)

  • Updated from 1.5.0 to 3.6.0 (this is just the next version, but Red Hat did quite the version bump there)
  • Added 'v' to the version; it is used by oc cluster up to determine which image should be downloaded.
  • Added myself as a maintainer.
Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

- Updated from 1.5.0 to 3.6.0 (this is just the next version, but Red
  Hat did quite the version bump there)
- Added 'v' to the version; it is used by `oc cluster up` to determine
  which image should be downloaded.
- Added myself as a maintainer.
@mention-bot
Copy link

@moretea, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bachp, @offlinehacker and @Mic92 to be potential reviewers.

The command `oc cluster up` mainly runs code though Docker containers.

However, in pkg/bootstrap/docker/host/host.go, nsenter is used to run
some commands on the host. For this to work on NixOS, we need to provide
the absolute path to the required programs.
@moretea
Copy link
Contributor Author

moretea commented Aug 18, 2017

@offlinehacker @bachp could you take a look?

@bachp
Copy link
Member

bachp commented Aug 18, 2017

Changes look good to me. I never tried oc cluster up on NixOS so far but I like that you were able to get
it working 👍


substituteInPlace pkg/bootstrap/docker/host/host.go \
--replace 'nsenter --mount=/rootfs/proc/1/ns/mnt mkdir' \
'nsenter --mount=/rootfs/proc/1/ns/mnt ${coreutils}/bin/mount'
Copy link
Member

Choose a reason for hiding this comment

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

mount is in util-linux only

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, thanks.

};

buildInputs = [ which ];

goPackagePath = null;
patchPhase = ''
patchShebangs ./hack
substituteInPlace pkg/bootstrap/docker/host/host.go \
--replace 'nsenter --mount=/rootfs/proc/1/ns/mnt findmnt' \
'nsenter --mount=/rootfs/proc/1/ns/mnt ${utillinux}/bin/findmnt'
Copy link
Member

Choose a reason for hiding this comment

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

Please also make replace nsenter with ${utillinux}/bin/nsenter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That will break stuff! These nsenter's are run from within the openshift container. It enters the namespace of the host of the container, and then executes e.g. findmt, mount & mkdir in there.

@joachifm
Copy link
Contributor

joachifm commented Sep 9, 2017

@moretea still working on this? I think @Mic92's points should be addressed before the PR can be merged.

@moretea
Copy link
Contributor Author

moretea commented Sep 10, 2017

I think that solves all of the issues.

@bachp
Copy link
Member

bachp commented Sep 10, 2017

I only tested the oc client command, but they work fine. 👍

@Mic92 Mic92 merged commit 9366d01 into NixOS:master Sep 14, 2017
@Mic92
Copy link
Member

Mic92 commented Sep 14, 2017

It was broken before right? So maybe I should backport it to stable?

@bachp
Copy link
Member

bachp commented Sep 14, 2017

@Mic92 The command line part, which is the only part I used so far was always working. At least I never had issues.

On the other hand having 3.6.0 in stable would also be good :)

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.

None yet

5 participants