Port build_ds_container.sh to python #7987

rhmdnd · 2021-12-09T20:43:08Z

The build_ds_container.sh script is useful for building content and
integrating it into your kubernetes clusters. Overtime, it's grown more
and more functionality, like building content in the cluster, locally,
or pushing built container images to repositories.

This commit attempts to reorganize the script and ports it to Python,
based on recommendations from the initial authors.

The primary changes include:

Using python kubernetes clients for interacting with clusters
Quieter logging by default, while maintaining verbosity using --debug
Auto-generated container image tags when publishing content to
repositories (useful for development workflows)

This patch doesn't include support for OCP deployments as it was
originally developed to build content for EKS benchmarks. That
functionality will come in another revision of this patch and should
remain backwards compatible with build_ds_container.sh.

Signed-off-by: Lance Bragstad lbragsta@redhat.com

Description:

Description here. Replace this text. Don't use the italics format!

Rationale:

Rationale here. Replace this text. Don't use the italics format!
Fixes # Issue number here (e.g. Updating sysctl XCCDF naming #26) or remove this line if no issue exists.

Vincent056 · 2021-12-10T02:19:01Z

utils/build_ds_container.py

+            api.create_namespace(body)
+            log.info('Created namespace %s', namespace)
+        except exceptions.ApiException as err:
+            if err.status == 409:


Just curious, does 409 always only refer to when a namespace already exists.

Yeah, that's AlreadyExists. Might be good to use a constant or comment this number.

It should only be emitted when the namespace already exists. I added a comment to the code to make that more clear.

Vincent056 · 2021-12-10T02:34:17Z

utils/build_ds_container.py

+
+def build_content_locally(products, debug=False):
+    build_binary_path = os.path.join(REPO_PATH, 'build_product')
+    command = [build_binary_path] + products


I haven't tried the code, but I am wondering do we need space between [build_binary_path] and products

Ok, I see, this is an array

Good question - it can be done either way in python. The python community issued guidance via a PEP that recommends using whitespace around operators.

https://www.python.org/dev/peps/pep-0008/#other-recommendations

Vincent056 · 2021-12-10T02:36:04Z

I think the codes looks good!

mrogers950 · 2021-12-10T16:53:29Z

utils/build_ds_container.py

+            return True
+        except exceptions.ApiException as err:
+            return False
+            if err.status == 404:


Hmm, can we reach this if the previous line is return False ?

Yep - good catch. I didn't finish smoothing out the wrinkles here before pushing this version.

I removed the if statement since I don't think it matters. Instead, I just logged something for the user and returned false.

This will be fixed in the next revision.

mrogers950 · 2021-12-10T16:59:05Z

utils/build_ds_container.py

+
+def create_image_stream():
+    # FIXME(lbragstad) Fill this out using an openshift client, since we're
+    # dealing with an openshift-specific resource.


Just wondering what we might gain by adding a second client lib for this. I think as long as the below works by specifying the api_version we'd be good.

mrogers950 · 2021-12-10T17:22:52Z

utils/build_ds_container.py

+    if result.returncode == 0:
+        log.info(f'Pushed image to {repository}:{tag}')
+    elif result.returncode == 125:
+        log.error(


Responded in the slack thread with some thoughts on the repo push.. It's unclear to me how the old script inherits repo and image tags to push this to the cluster.

@JAORMX added that functionality recently as command line arguments. In that implementation, the script relied on podman login.

I initially implemented this method with the ability to pass in a username/password and invoking the command with --creds, but afterwards I removed it to keep things simpler.

Instead, @JAORMX and I agreed that we can leave authentication to the user, prior to them running this script. In that case, I think the best we can do is just detect if authentication failed for some reason and give them guidance on how to fix it.

utils/build_ds_container.py

The build_ds_container.sh script is useful for building content and integrating it into your kubernetes clusters. Overtime, it's grown more and more functionality, like building content in the cluster, locally, or pushing built container images to repositories. This commit attempts to reorganize the script and ports it to Python, based on recommendations from the initial authors. The primary changes include: - Quieter logging by default, while maintaining verbosity using --debug - Auto-generated container image tags when publishing content to repositories (useful for development workflows) Signed-off-by: Lance Bragstad <lbragsta@redhat.com>

Since we ported the build_ds_container.sh script to a python equivalent, we don't need to maintain both versions and introduce potential drift. This commit updates the bash script to print a message for users to adopt the new script.

Vincent056

Just tested out the python code, I am wondering if we could have a more verbose output for example when Building content for ocp4, rhcos4, in case of errors?

rhmdnd · 2022-01-18T21:03:47Z

@Vincent056 I suppressed the output by default, but it should emit the same output as the bash script if you pass --debug, but let me know if this isn't working how you expect it to.

Vincent056 · 2022-01-18T21:23:06Z

@Vincent056 I suppressed the output by default, but it should emit the same output as the bash script if you pass --debug, but let me know if this isn't working how you expect it to.

Sorry didn't see that earlier, it looks good!

rhmdnd · 2022-01-18T23:00:30Z

@Vincent056 I can take the opposite approach too and emit verbose logging by default, and supply an option to suppress logging using something like --quiet if you prefer.

jhrozek

The quiet mode was a bit of a surprise to me at first (I happened to have a broken profile in tree..) but I think it's fine and we can revert the debug option to quiet later on.
/lgtm

The requested changes were addressed

openshift-ci bot added the do-not-merge/work-in-progress Used by openshift-ci bot. label Dec 9, 2021

Vincent056 reviewed Dec 10, 2021

View reviewed changes

mrogers950 reviewed Dec 10, 2021

View reviewed changes

rhmdnd force-pushed the port-build-ds-container-to-python branch 2 times, most recently from a0c378d to 806c914 Compare December 10, 2021 19:11

JAORMX previously requested changes Dec 14, 2021

View reviewed changes

utils/build_ds_container.py Outdated Show resolved Hide resolved

rhmdnd force-pushed the port-build-ds-container-to-python branch from 806c914 to 757476f Compare January 17, 2022 22:56

rhmdnd changed the title ~~WIP: Port build_ds_container.sh to python~~ Port build_ds_container.sh to python Jan 17, 2022

openshift-ci bot removed the do-not-merge/work-in-progress Used by openshift-ci bot. label Jan 17, 2022

rhmdnd added 2 commits January 18, 2022 12:10

Deprecated build_ds_container.sh

0186161

Since we ported the build_ds_container.sh script to a python equivalent, we don't need to maintain both versions and introduce potential drift. This commit updates the bash script to print a message for users to adopt the new script.

rhmdnd force-pushed the port-build-ds-container-to-python branch from 757476f to 0186161 Compare January 18, 2022 18:12

Vincent056 reviewed Jan 18, 2022

View reviewed changes

jhrozek approved these changes Jan 27, 2022

View reviewed changes

jhrozek merged commit 0a818b7 into ComplianceAsCode:master Jan 27, 2022

yuumasato added this to the 0.1.61 milestone Jan 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port build_ds_container.sh to python #7987

Port build_ds_container.sh to python #7987

rhmdnd commented Dec 9, 2021

Vincent056 Dec 10, 2021

mrogers950 Dec 10, 2021

rhmdnd Dec 10, 2021

Vincent056 Dec 10, 2021

Vincent056 Dec 10, 2021

rhmdnd Dec 10, 2021

Vincent056 commented Dec 10, 2021

mrogers950 Dec 10, 2021

rhmdnd Dec 10, 2021 •

edited

Loading

mrogers950 Dec 10, 2021

rhmdnd Dec 10, 2021

mrogers950 Dec 10, 2021

rhmdnd Dec 10, 2021

Vincent056 left a comment

rhmdnd commented Jan 18, 2022 •

edited

Loading

Vincent056 commented Jan 18, 2022

rhmdnd commented Jan 18, 2022

jhrozek left a comment

Port build_ds_container.sh to python #7987

Port build_ds_container.sh to python #7987

Conversation

rhmdnd commented Dec 9, 2021

Description:

Rationale:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vincent056 commented Dec 10, 2021

Choose a reason for hiding this comment

rhmdnd Dec 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vincent056 left a comment

Choose a reason for hiding this comment

rhmdnd commented Jan 18, 2022 • edited Loading

Vincent056 commented Jan 18, 2022

rhmdnd commented Jan 18, 2022

jhrozek left a comment

Choose a reason for hiding this comment

rhmdnd Dec 10, 2021 •

edited

Loading

rhmdnd commented Jan 18, 2022 •

edited

Loading