Skip to content

Conversation

@camrynl
Copy link
Contributor

@camrynl camrynl commented Aug 11, 2022

Reason for Change:

update cni.Dockerfile to pack released version of azure-vnet instead of local version.

Issue Fixed:

Requirements:

Notes:

@camrynl camrynl marked this pull request as draft August 11, 2022 17:41
@camrynl
Copy link
Contributor Author

camrynl commented Aug 11, 2022

completing in parts since there isn't a release for azure-ipam yet. later I'd like to discuss how to create the release.

@camrynl camrynl marked this pull request as ready for review August 11, 2022 22:46
@camrynl camrynl requested a review from rbtr August 11, 2022 22:46
rbtr
rbtr previously approved these changes Aug 12, 2022
Copy link
Collaborator

@rbtr rbtr left a comment

Choose a reason for hiding this comment

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

looks good to me, ty 🚀

ARG VERSION
ARG OS
ARG ARCH
ENV OS=${OS}
Copy link
Collaborator

Choose a reason for hiding this comment

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

do you need to reassign the arg to an env variable like this? is it because we use it in a RUN?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, just ran make without the env variable assignment and was able to build without those lines. removed it from the pr

rbtr
rbtr previously approved these changes Aug 12, 2022
COPY dropgz .
COPY --from=azure-ipam /azure-ipam/*.conflist pkg/embed/fs
COPY --from=azure-ipam /azure-ipam/bin/* pkg/embed/fs
COPY --from=azure-vnet /azure-container-networking/cni/*.conflist pkg/embed/fs
Copy link
Collaborator

Choose a reason for hiding this comment

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

it would be nice to parameterize the conflist copy by $OS and copy to a generic name (eg azure-linux-swift.conflist -> azure-swift.conflist) to make the installation that little bit more platform-agnostic

@rbtr rbtr enabled auto-merge (squash) August 19, 2022 18:18
@rbtr rbtr merged commit 6ef6319 into Azure:master Aug 19, 2022
@camrynl camrynl deleted the cniInstaller branch August 19, 2022 20:35
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