Skip to content

Conversation

20100M
Copy link

@20100M 20100M commented Mar 1, 2023

An example build file to create a sandbox environment to experiment with the c++ and python bindings, and run validation

@@ -0,0 +1,38 @@
FROM centos:latest
Copy link
Collaborator

Choose a reason for hiding this comment

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

does it have to be centos? can this run on a smaller distro like alpine?

Copy link
Author

Choose a reason for hiding this comment

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

I chose centos out of personal convenience, it doesn't have to be

Copy link
Collaborator

Choose a reason for hiding this comment

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

i should mention that this kinda depends on the usage; for demos, id say use alpine to make it smaller for distribution. for dev envs, centos can be fine because you'd normally build the container locally

#
# paste this in ocpdiag.docker
#
# docker build --rm ocpdiag.docker centos:ocpdiag
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this meant as a dev tool or demo?
if former, it would be better to use the more usual docker file structure <proj name>/Dockerfile, and that can be expanded on later. and move it somewhere else other than the ./docs folder (/tools or something maybe?)

if latter, id move it do ./docs/demos/ocpdiag.docker and put something else as entrypoint to showcase the demo, rather than a shell

Copy link
Author

Choose a reason for hiding this comment

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

I put this as a draft PR rather than shared as a code block in our other communication channels for ease of access by others. My intent was that when I went on to try to build a bare-bones "HelloWorld/SmokeTest" test for the quick start guide, I encountered an error with the bazel build step, I figured the easiest would be to share a reproducible environment for others to review the error.

I'm thinking more of a dev tool but something to discuss, not in its current form.

I used the docs folder as I figured we could have a doc section supporting examples on how to adopt and docker could be one way to onboard folks.

The entrypoint used is because I'm looking at this more as a runtime environment than a service or demo

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok, this wasnt clear to me. there was no message in the PR saying that this is just a request for comments thing and will not actually be merged

Copy link
Author

Choose a reason for hiding this comment

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

This was the first time I created a draft PR, https://github.blog/2019-02-14-introducing-draft-pull-requests/
it is a bit cryptic, I can see it tagged draft and at the bottom it has a button with "ready for review" but I agree it's not quite intuitive

#
###############################################################

RUN sed -i 's/mirrorlist/#mirrorlist/g' /etc/yum.repos.d/CentOS-*
Copy link
Collaborator

Choose a reason for hiding this comment

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

comments on these? why are they removed?

Copy link
Author

Choose a reason for hiding this comment

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

this tweak is needed to circumvent docker bailing out at build time

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok but why?
what i was saying was that it's not obvious why you do this, so it would be useful to add a comment in the docker file explaining

Copy link
Author

Choose a reason for hiding this comment

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

I hear you and will add a comment if we decide to move this dockerfile forward. The reason is that centos 8 EOL'd and the mirrors were moved to vault.centos.org.

RUN cd / && unzip ocp-diag-core-main.zip

WORKDIR /ocp-diag-core-main
RUN python3 ./apis/python/examples/demo.py 2>&1 > /ocpdiag_py_demo.log
Copy link
Collaborator

Choose a reason for hiding this comment

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

this seems like a rather odd usage of run commands; these are meant to build the image, but i get the feeling youre using them here as a demo (which also answers my question above). so by doing this youre baking in the results of these commands into the image, which has various implications.

what i think you should do is to add all of this to a shell script, pull that into the image and then use it as entrypoint (which may end up in the interactive shell if that's a goal)
this way, the image stays clean of artifacts that variate (like these log outputs) and docker can reuse it without rebuilding every time

Copy link
Author

Choose a reason for hiding this comment

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

Your prompt review of this beat me to setting up context, hopefully my explanation on the intent in an earlier comment explains how I approached this and why I put it out here as a draft.

So, yes, those executions are very much not intended as part of the build file and more of an example so others can reproduce the build error.

If deemed worthwhile I could see the validator become an image and the run of it could be parametrized to take test output as input

Copy link
Collaborator

Choose a reason for hiding this comment

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

let's revisit this after @dhawkes tells us about the dev docker file

@mimir-d
Copy link
Collaborator

mimir-d commented Mar 1, 2023

youre also missing the --sign-off in your commits; so the DCO check fails

@mimir-d
Copy link
Collaborator

mimir-d commented Mar 1, 2023

and as more guidance, please have a look here re. commit messages
https://cbea.ms/git-commit/

the minimal should be 1. a title, 2. some context message and what was done

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