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

container: self contained build script #63

Merged

Conversation

phlogistonjohn
Copy link
Collaborator

Right now, sambacc and samba-containers is in a pretty awkward situation. The base oses for the images are different and we're building RPMs that have the wrong ABI for the samba-container images.

These changes pull nearly all the logic out of the continerfile and into build.sh so that it is easier to build and test and varying base OSes. If this is merged we have the ability to create multi-stage builds in samba-containers project hat pretty much only need to consume sambacc's container image OR even simply drop that and do something like curl build.sh from sambacc's github repo. Regardless of how we use it in the samba-containers repo I think this is helpful step to get out of a bad sitation and gain flexibility when executing tests.


This now makes it possible to more easily test and reuse the sambacc
test (and build) container image for different base OSes. For example:

podman build -t sambacc:mini36 --build-arg=SAMBACC_BASE_IMAGE=registry.fedoraproject.org/fedora:36 --build-arg=SAMBACC_MINIMAL=yes -f tests/container/Containerfile
podman run --rm -it sambacc:mini36

podman build -t sambacc:c9s -f tests/container/Containerfile

podman build -t sambacc:mini9 --build-arg=SAMBACC_MINIMAL=yes -f tests/container/Containerfile

podman build -t sambacc:fc37 --build-arg=SAMBACC_BASE_IMAGE=registry.fedoraproject.org/fedora:37 -f tests/container/Containerfile

@phlogistonjohn
Copy link
Collaborator Author

I forgot to mention I don't think there's a quick fix in samba-containers by simply converting everything that uses sambacc to centos:stream9 because AFAIK there's no dc package so we simply can't convert the ad dc image to centos at the moment. I will follow up this PR with a PR on samba-containers that let's us remain on fedora for the ad dc image.

Copy link
Contributor

@obnoxxx obnoxxx left a comment

Choose a reason for hiding this comment

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

just a small suggestion ...

tests/container/Containerfile Outdated Show resolved Hide resolved
obnoxxx
obnoxxx previously approved these changes Jan 26, 2023
Copy link
Contributor

@obnoxxx obnoxxx left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

@obnoxxx
Copy link
Contributor

obnoxxx commented Jan 26, 2023

@anoopcs9 could you please review this so that we have an opportunity to see mergify in action? :-)

@anoopcs9
Copy link
Collaborator

anoopcs9 commented Jan 27, 2023

I will follow up this PR with a PR on samba-containers that let's us remain on fedora for the ad dc image.

How do we deal with different Samba package versions in server and ad-dc image?

@anoopcs9
Copy link
Collaborator

I will follow up this PR with a PR on samba-containers that let's us remain on fedora for the ad dc image.

How do we deal with different Samba package versions in server and ad-dc image?

To be more clearer, I was thinking on how those server and ad-dc images would be consumed in deploying samba-operator. If we plan to maintain same version this would mean that we create specific tags for sambacc and samba-container like fc36, c9s etc. In that case why don't we just live with what we had earlier i.e, by reverting #60?

@obnoxxx obnoxxx requested a review from gd January 27, 2023 13:59
@phlogistonjohn
Copy link
Collaborator Author

phlogistonjohn commented Jan 27, 2023

I will follow up this PR with a PR on samba-containers that let's us remain on fedora for the ad dc image.

How do we deal with different Samba package versions in server and ad-dc image?

To be more clearer, I was thinking on how those server and ad-dc images would be consumed in deploying samba-operator. If we plan to maintain same version this would mean that we create specific tags for sambacc and samba-container like fc36, c9s etc. In that case why don't we just live with what we had earlier i.e, by reverting #60?

I see making some progress to using centos as a base image worthwhile, even if we made a bit of a mess on our way there. The fact is, while I'd love the ad dc image to be something useful for a wide variety of production-like use cases, the ad dc image is "merely" used by our test frameworks for verifying AD related features in samba-operator at this time.

Since samba-operator never deploys ad dc images, all of our "production ready" bits can be on centos. We can and should work towards getting ad dc on the same base, but I don't think we have to do it all at once. I would much prefer to use the same base for everything, but since that doesn't work at this time something's gotta give.

@obnoxxx obnoxxx requested a review from spuiuk January 30, 2023 09:52
@anoopcs9
Copy link
Collaborator

I will follow up this PR with a PR on samba-containers that let's us remain on fedora for the ad dc image.

How do we deal with different Samba package versions in server and ad-dc image?

To be more clearer, I was thinking on how those server and ad-dc images would be consumed in deploying samba-operator. If we plan to maintain same version this would mean that we create specific tags for sambacc and samba-container like fc36, c9s etc. In that case why don't we just live with what we had earlier i.e, by reverting #60?

I see making some progress to using centos as a base image worthwhile, even if we made a bit of a mess on our way there. The fact is, while I'd love the ad dc image to be something useful for a wide variety of production-like use cases, the ad dc image is "merely" used by our test frameworks for verifying AD related features in samba-operator at this time.

Since samba-operator never deploys ad dc images, all of our "production ready" bits can be on centos. We can and should work towards getting ad dc on the same base, but I don't think we have to do it all at once. I would much prefer to use the same base for everything, but since that doesn't work at this time something's gotta give.

Even in that case I don't know if this is the right approach. I made an attempt to make things work back again by allowing el9 build of python3-sambacc to be installed on f36. With some extra configuration it may succeed finally. I know that we then end up with f36 based server image which I am OK to live with for some more time.

@mergify
Copy link

mergify bot commented Jan 30, 2023

This pull request now has conflicts with the target branch. Please resolve these conflicts and force push the updated branch.

Most of the logic around building and testing sambacc was in the
build.sh script already, so why not include the ability to install
the os packages too.

When called with `--install` as the first argument instead of a revision
id, we assume that the script is being called specifically and only to
install the package dependencies.

Signed-off-by: John Mulligan <[email protected]>
Strip down the containerfile to the bare minimum, the base image, the
build.sh script, and dependencies. Make the base image a build arg.
Make the installation of the dependencies optional based on a build arg.

This now makes it possible to more easily test and reuse the sambacc
test (and build) container image for different base OSes. For example:

```
podman build -t sambacc:mini36 --build-arg=SAMBACC_BASE_IMAGE=registry.fedoraproject.org/fedora:36 --build-arg=SAMBACC_MINIMAL=yes -f tests/container/Containerfile
podman run --rm -it sambacc:mini36

podman build -t sambacc:c9s -f tests/container/Containerfile

podman build -t sambacc:mini9 --build-arg=SAMBACC_MINIMAL=yes -f tests/container/Containerfile

podman build -t sambacc:fc37 --build-arg=SAMBACC_BASE_IMAGE=registry.fedoraproject.org/fedora:37 -f tests/container/Containerfile
```

Signed-off-by: John Mulligan <[email protected]>
@mergify mergify bot dismissed obnoxxx’s stale review January 30, 2023 18:02

Pull request has been modified.

Copy link
Contributor

@obnoxxx obnoxxx left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Collaborator

@anoopcs9 anoopcs9 left a comment

Choose a reason for hiding this comment

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

Forgot to mark the approval.

@mergify mergify bot merged commit 9d8b892 into samba-in-kubernetes:master Feb 1, 2023
@phlogistonjohn
Copy link
Collaborator Author

Thanks for the approval. I feel like I'm doing a poor job of explaining how we currently build sambacc & how I envision these changes being used. Maybe if we do a voice meeting soon I can try doing a better job then.

@phlogistonjohn phlogistonjohn deleted the jjm-self-contained-build branch March 3, 2023 14:30
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.

3 participants