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

Ensure git submodules work with Openshift [#340] #341

Merged
merged 3 commits into from Feb 12, 2019

Conversation

Projects
None yet
4 participants
@darkmuggle
Copy link
Contributor

darkmuggle commented Feb 11, 2019

Some versions of Openshift [1] do not include .git when building Docker containers [2]. In [3], we discovered that Git submodules are also affected. Further, since mantle run git describe --dirty, check on its source is insufficient.

This is a follow-on PR to #320 that resolves #340.

@darkmuggle darkmuggle added the WIP label Feb 11, 2019

@ashcrow
Copy link
Collaborator

ashcrow left a comment

Looks good so far

@darkmuggle darkmuggle force-pushed the darkmuggle:issue-340 branch from 0e98db7 to 1151835 Feb 12, 2019

@darkmuggle

This comment has been minimized.

Copy link
Contributor Author

darkmuggle commented Feb 12, 2019

Here's how to test this locally:

cat > Dockerfile.test <<EOF
FROM qauy.io/coreos/coreos-assembler
USER root
RUN rm -rf /root/containerbuild
WORKDIR /root/containerbuild
ENV OPENSHIFT_GIT_HACK=1
ENV GIT_URL=https://github.com/darkmuggle/coreos-assembler
ENV GIT_REF=issue-340
COPY ./ /root/containerbuild/
RUN rm -rf /root/containerbuild/.git
RUN ./build.sh write_archive_info
RUN ./build.sh make_and_makeinstall
RUN ./build.sh configure_user
WORKDIR /srv/
RUN rm -rf /root/containerbuild
RUN chmod g=u /etc/passwd
USER builder
ENTRYPOINT ["/usr/bin/dumb-init", "scl", "enable", "rh-python36", "--", "/usr/bin/coreos-assembler"]
EOF

docker build -f Dockerfile.test -t localtest .

@darkmuggle darkmuggle requested review from dustymabe and cgwalters Feb 12, 2019

Show resolved Hide resolved src/cmdlib.sh Outdated
@ashcrow

This comment has been minimized.

Copy link
Collaborator

ashcrow commented Feb 12, 2019

Still WIP?

@darkmuggle darkmuggle force-pushed the darkmuggle:issue-340 branch from 1151835 to 0c7081f Feb 12, 2019

@darkmuggle

This comment has been minimized.

Copy link
Contributor Author

darkmuggle commented Feb 12, 2019

Waiting on an Openshift build. I'm close. Stripping .git/* out is really a user-hostile action on Openshift's part (IHMO).

@darkmuggle darkmuggle force-pushed the darkmuggle:issue-340 branch from 0c7081f to bcd0d40 Feb 12, 2019

@darkmuggle

This comment has been minimized.

Copy link
Contributor Author

darkmuggle commented Feb 12, 2019

Okay, its ready for review. Builds in local podman, Openshift and Quay all work.

https://quay.io/repository/behoward/coreos-assembler/build/7f8371a5-6538-46e3-93b4-16670e119d85

Removing WIP tag.

@darkmuggle darkmuggle removed the WIP label Feb 12, 2019

@darkmuggle

This comment has been minimized.

Copy link
Contributor Author

darkmuggle commented Feb 12, 2019

@cgwalters @ashcrow, please review (and if you feel appropriate, merge) at your earliest connivence.

popd
fi

rsync -av --ignore-existing "${tmpgit}"/clone/ "${gitd}"/

This comment has been minimized.

@darkmuggle

darkmuggle Feb 12, 2019

Author Contributor

For the record, I strongly dislike this approach, but its the least bad solution that ensures that only missing files are added back, under the assumption it will only be hit in a CI solution (need to have envVars OPENSHIFT_GIT_HACK, GIT_REF and GIT_URL specifically defined). But this is the only real option with EL7 since the git submodule doesn't support changing the work tree, and otherwise the submodules lose their git state.

This comment has been minimized.

@ashcrow

ashcrow Feb 12, 2019

Collaborator

Understood

This comment has been minimized.

@dustymabe

dustymabe Feb 12, 2019

Collaborator

can we add a comment above this rsync with some of that context?

This comment has been minimized.

@darkmuggle

darkmuggle Feb 12, 2019

Author Contributor

@dustymabe I re-worked the the comments, rebased and pushed.

@miabbott
Copy link
Contributor

miabbott left a comment

It'll be nice when we can stop working around EL7

@ashcrow
Copy link
Collaborator

ashcrow left a comment

LGTM

popd
fi

rsync -av --ignore-existing "${tmpgit}"/clone/ "${gitd}"/

This comment has been minimized.

@ashcrow

ashcrow Feb 12, 2019

Collaborator

Understood

@ashcrow

This comment has been minimized.

Copy link
Collaborator

ashcrow commented Feb 12, 2019

@dustymabe mind taking a look as well? I'd like one more person who has been more so in the development of cosa to review before we merge.

@dustymabe

This comment has been minimized.

Copy link
Collaborator

dustymabe commented Feb 12, 2019

one nit comment - otherwise LGTM

@darkmuggle darkmuggle force-pushed the darkmuggle:issue-340 branch from bcd0d40 to 991107d Feb 12, 2019

# $OPENSHIFT_GIT_HACK, $GIT_REF and $GIT_URL are environment variables,
# provided by Openshift or the container run time. See
# https://github.com/coreos/coreos-assembler/pull/324 for an example.
# See https://github.com/coreos/coreos-assembler/issues/320 and

This comment has been minimized.

@darkmuggle

darkmuggle Feb 12, 2019

Author Contributor

@dustymabe okay, I put more documentation in regarding the context. How does this look?

This comment has been minimized.

@dustymabe

dustymabe Feb 12, 2019

Collaborator

LGTM

# source; rsync is less than ideal, but it allows a work-around for the combination
# of RHEL 7 and problematic Openshift versions.

# This hack is SHOULD ONLY BE HIT WHEN RUNNING IN A CI SITUATION, under

This comment has been minimized.

@dustymabe

dustymabe Feb 12, 2019

Collaborator

hack is SHOULD -> hack SHOULD

@dustymabe

This comment has been minimized.

Copy link
Collaborator

dustymabe commented Feb 12, 2019

merge away when you're ready

darkmuggle added some commits Feb 11, 2019

src/cmdlib.sh: checkout of git submodules for Openshift hack [#340].
In follow-up on #320, this ensures that Openshift builds re-initialize
the git submodules too.

@darkmuggle darkmuggle force-pushed the darkmuggle:issue-340 branch from 991107d to b0b127d Feb 12, 2019

@darkmuggle darkmuggle merged commit 460741e into coreos:master Feb 12, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@darkmuggle darkmuggle deleted the darkmuggle:issue-340 branch Feb 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment