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

Tickets/dm36232: Install ts-integrationtests and robotframework_EFD to integrationtests image #148

Merged
merged 5 commits into from
Sep 23, 2022

Conversation

rbovill
Copy link
Contributor

@rbovill rbovill commented Sep 19, 2022

No description provided.

Copy link
Contributor

@mareuter mareuter left a comment

Choose a reason for hiding this comment

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

We need to have a talk about this container. Somethings got lost translation.

build/integrationtests/Dockerfile Outdated Show resolved Hide resolved
build/integrationtests/Dockerfile Outdated Show resolved Hide resolved
build/integrationtests/Dockerfile Outdated Show resolved Hide resolved
build/integrationtests/Dockerfile Outdated Show resolved Hide resolved
build/integrationtests/Dockerfile Outdated Show resolved Hide resolved
build/integrationtests/Dockerfile Show resolved Hide resolved
build/integrationtests/Dockerfile Outdated Show resolved Hide resolved
build/integrationtests/Dockerfile Outdated Show resolved Hide resolved
build/integrationtests/Dockerfile Outdated Show resolved Hide resolved
build/integrationtests/README Outdated Show resolved Hide resolved
@rbovill rbovill force-pushed the tickets/DM-36232 branch 3 times, most recently from 23d1c4a to af07aef Compare September 21, 2022 16:41
@tribeiro
Copy link
Member

I just merged my PR, you may want to rebase your branch to main before merging the PR.. Also, remember to update the version history.

…ests and robotframework conda packages and checking out the robotframework_EFD repo.

Explicitly checking out the main and develop branches of the robotframework_EFD repo.
…e-build conventions and removed obsolete steps.

Added a startup.sh script.

Updated the README to accurately document the Dockerfile.
@@ -1,23 +1,29 @@
ARG cycle
ARG hub
ARG integrationtests
Copy link
Member

Choose a reason for hiding this comment

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

If I remember correctly, all the args defined before the "FROM" clause are wiped out just after it.

Also, it is best if you use the ARGS close to where you need it which, in this case, is just after line 13...

This makes sure the layers are better reused when you change their values...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

LABEL maintainer Rob Bovill <rbovill@lsst.org>
LABEL integrationtests=${integrationtests}
Copy link
Member

Choose a reason for hiding this comment

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

Consider adding this LABEL at the end of the build, or at least after you install the package.. This helps docker build reuse layers.

You can see how we do on the other build, e.g.:

LABEL ts_idl=${ts_idl} \
ts_observatory_control=${ts_observatory_control} \
ts_config_attcs=${config} \
ts_ataos=${ataos}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


LABEL integrationtests=${integrationtests}
WORKDIR ${HOME}/robotframework_EFD
RUN ${HOME}/.checkout_repo.sh main develop
Copy link
Member

Choose a reason for hiding this comment

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

you just cloned the repo a couple lines above, do you need to do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess we'll only want main for the actual executions. Removed.

build/integrationtests/startup.sh Show resolved Hide resolved
…ture; right now it is just dummy command. The RUN_ARG value will be updated once we determine what it should do.
Copy link
Member

@tribeiro tribeiro 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...

@rbovill rbovill merged commit 420df01 into main Sep 23, 2022
@rbovill rbovill deleted the tickets/DM-36232 branch September 23, 2022 20:53
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