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

Docker build #65

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Conversation

SmithPeder
Copy link
Contributor

No description provided.

@SmithPeder SmithPeder force-pushed the docker-build branch 22 times, most recently from ac0d022 to 44655a1 Compare February 16, 2021 22:17
Copy link
Contributor

@odinuge odinuge left a comment

Choose a reason for hiding this comment

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

Can you split this into multiple logical PRs instead? In its current state it has 41 changed files, and is doing multiple independent things at the same time..

I would at least like to don't move the tex files in this PR, and make a separate one do that, to make it easy to see that this is not changing the content. It will also break git blame, so imo. we should not do unless we have a good reason to do it....

cp public/*.pdf public/archive/$TIMESTAMP.pdf

# Clean up the Statutter directory, as it used to genereate fles
rm -f ./statutter/*.log \
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to remove these files?

@@ -0,0 +1,33 @@
TIMESTAMP=`eval date +%d.%B.%Y`
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need a shell script to generate them, instead of the Makefile?

If we have a shell-script, it should at least do proper error handling to make it possible to understand if something fails...

Copy link
Contributor

Choose a reason for hiding this comment

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

We should at least enforce shellcheck

event:
- pull_request

- name: push
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this doing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't work yet, but it's supposed to do the same things as Travis does, where it builds then pushes the public code to another branch, which is served.

The whole process isn't planned yet.


Den mounter de tre volumene `statutter, static og public` som blir brukt av bildet når den bygger statutter.

I tillegg kan du sende inn `ASSEMBLY_DATE` som en variabel, og den genererte pdf filen som blir arkivert får dette navnet.
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this env-var doing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was a thing back in the day, but now it's deprecated.

-v `pwd`/static:/static/ \
-v `pwd`/public:/public/ \
-e ASSEMBLY_DATE=01.januar.1970 \
abakus/statuttbuilder:latest
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it better to use the Dockerfile instead, to make sure that the correct version is used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That might be a good idea

@SmithPeder SmithPeder marked this pull request as draft February 17, 2021 12:46
@SmithPeder
Copy link
Contributor Author

All valid questions :) I was supposed to set this as a draft, just wanted to get the .drone to do the build to see if everything works as planned. The scripts/README is kinda outdated, sorry.

@LudvigHz
Copy link
Member

An idea I had that might be better than committing the pdfs, is to attach them as release files on push to master (and create a new release). I don't know if it's much better or worse, but I think it's a bit cleaner and it might be a better way to keep a history. @SmithPeder

This way the different versions (releases) are more directly associated with a commit as well.
Thoughts?

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