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

ARG build variables: django, channels, daphne #1036

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

romer8
Copy link
Contributor

@romer8 romer8 commented Apr 14, 2024

  • ARG build variables to support versions Django, django channels, and daphne according to user needs
  • enviroment.yml and micro_environment.yml are edited in line according to the ARG build env variables

@coveralls
Copy link

Coverage Status

coverage: 100.0%. remained the same
when pulling 2e73800 on romer8:django_versions
into 9d5c55b on tethysplatform:main.

Dockerfile Outdated
@@ -136,6 +139,22 @@ COPY --chown=$MAMBA_USER:$MAMBA_USER micro_environment.yml ${TETHYS_HOME}/tethys

WORKDIR ${TETHYS_HOME}/tethys

# Set the versions of Django, Channels, and Daphne if provided in environment.tyml and micro_environment.yml
RUN if [ -n "$DJANGO_VERSION" ]; then \
sed -i "s/- django>=3.2,<6/- django==${DJANGO_VERSION}/" environment.yml micro_environment.yml; \
Copy link
Member

Choose a reason for hiding this comment

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

Can the version part of the django be found using a regular expression? Try this to see if it works:

Suggested change
sed -i "s/- django>=3.2,<6/- django==${DJANGO_VERSION}/" environment.yml micro_environment.yml; \
sed -i "s/- django.+/- django==${DJANGO_VERSION}/" environment.yml micro_environment.yml; \

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it can be actually with the code below now the sed command find - django, and it replaces it with the DJANGO_VERSION arg variable, but it also ignores anything -django- becuase there is some dependencies that contain django in the name followed by a -:
sed -i "s/\s*- django[^-].*/ - django==${DJANGO_VERSION}/" environment.yml micro_environment.yml;

Dockerfile Outdated
Comment on lines 148 to 149
else \
sed -i "s/- channels/- channels/" environment.yml micro_environment.yml; \
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this else is necessary is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it is not necessary, and it was removed

Dockerfile Outdated
Comment on lines 153 to 154
else \
sed -i "s/- daphne/- daphne/" environment.yml micro_environment.yml; \
Copy link
Member

Choose a reason for hiding this comment

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

The else probably isn't necessary here either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it is not necessary, and it was removed

Dockerfile Outdated
sed -i "s/- channels/- channels/" environment.yml micro_environment.yml; \
fi && \
if [ -n "$DAPHNE_VERSION" ]; then \
sed -i "s/- daphne/- daphne==${DAPHNE_VERSION}/" environment.yml micro_environment.yml; \
Copy link
Member

Choose a reason for hiding this comment

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

Should probably have a regular expression on this too:

Suggested change
sed -i "s/- daphne/- daphne==${DAPHNE_VERSION}/" environment.yml micro_environment.yml; \
sed -i "s/- daphne.+/- daphne==${DAPHNE_VERSION}/" environment.yml micro_environment.yml; \

Copy link
Contributor Author

@romer8 romer8 Apr 29, 2024

Choose a reason for hiding this comment

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

The following was changed to get anything after - daphne and replaced if arg build was set
sed -i "s/\s*- daphne.*/ - daphne==${DAPHNE_VERSION}/" environment.yml micro_environment.yml;

Dockerfile Outdated
sed -i "s/- django>=3.2,<6/- django==${DJANGO_VERSION}/" environment.yml micro_environment.yml; \
fi && \
if [ -n "$DJANGO_CHANNELS_VERSION" ]; then \
sed -i "s/- channels/- channels==${DJANGO_CHANNELS_VERSION}/" environment.yml micro_environment.yml; \
Copy link
Member

Choose a reason for hiding this comment

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

Should probably have a regular expression on this too:

Suggested change
sed -i "s/- channels/- channels==${DJANGO_CHANNELS_VERSION}/" environment.yml micro_environment.yml; \
sed -i "s/- channels.+/- channels==${DJANGO_CHANNELS_VERSION}/" environment.yml micro_environment.yml; \

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The following was changed to get anything after - channels and replaced if arg build was set
sed -i "s/\s*- channels.*/ - channels==${DJANGO_CHANNELS_VERSION}/" environment.yml micro_environment.yml;

Comment on lines +8 to +9
ARG DJANGO_CHANNELS_VERSION
ARG DAPHNE_VERSION
Copy link
Member

Choose a reason for hiding this comment

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

Can args be declared without a value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, they can be. They actually help with the if statement, if it is not defined then it will not enter the if statement

…ions, and also removed looking for specific version.
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.

None yet

3 participants