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

#227 build basic backend deb package #228

Merged
merged 52 commits into from
Feb 10, 2021
Merged

#227 build basic backend deb package #228

merged 52 commits into from
Feb 10, 2021

Conversation

elKei24
Copy link
Contributor

@elKei24 elKei24 commented Feb 4, 2021

Does not close issue #227 yet, but is a working intermediate step and prevents all other PRs from getting red.

Builds a basic .deb package containing the built backend and a systemd service file for starting it.

@elKei24 elKei24 self-assigned this Feb 4, 2021
@elKei24 elKei24 force-pushed the 227-deb-packages branch 15 times, most recently from 8864f1b to 821b204 Compare February 4, 2021 14:44
@elKei24 elKei24 force-pushed the 227-deb-packages branch 3 times, most recently from 91d674a to cb6cdb9 Compare February 4, 2021 15:28
@elKei24 elKei24 force-pushed the 227-deb-packages branch 2 times, most recently from f8e20a5 to e3c31a2 Compare February 6, 2021 11:50
@@ -2,11 +2,9 @@

exposed_version=0.28.1

app.postgres.host=localhost
app.postgres.port=5432
app.postgres.url=jdbc:postgresql://localhost:5432/ehrenamtskarte
Copy link
Member

Choose a reason for hiding this comment

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

didn't we have some problems with that?

Copy link
Member

Choose a reason for hiding this comment

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

This is equivalent to the previous way. I no longer concat the url in java but pass the whole one here.

Comment on lines 14 to 18
if (!production) {
cfg.enableDevLogging()
cfg.enableCorsForAllOrigins()
cfg.addStaticFiles("/graphiql", "/graphiql", Location.CLASSPATH)
}
Copy link
Member

Choose a reason for hiding this comment

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

do we disable graphiql in production? I'd probably vote for having it enabled :D

Copy link
Member

@maxammann maxammann Feb 9, 2021

Choose a reason for hiding this comment

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

I think this is a tool for debugging and should not be exposed in production. Also you can always use intellij for sending graphql queries.

Not sure whether this tool is intended for production.

Copy link
Member

Choose a reason for hiding this comment

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

To me, its just a public access to the api, which shouldn't do any harm...

Copy link
Member

Choose a reason for hiding this comment

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

I see this more like a standalone application which could be deployed to production. I mean it is like the administration backend. I added it again because there is no problem from security perspective as these are just static files.

<excludeFolder url="file://$MODULE_DIR$/backend" />
<excludeFolder url="file://$MODULE_DIR$/frontend" />
</content>
<content url="file://$MODULE_DIR$" />
Copy link
Member

Choose a reason for hiding this comment

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

Hmm... I think these changes should not be necessary....

Copy link
Member

Choose a reason for hiding this comment

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

They also do not harm :) i tested this. No differnce with or without these changes.


sub_filter_once off;
sub_filter_types application/json;
sub_filter "https://tiles.staging.ehrenamtskarte.app" "http://localhost:5002";
Copy link
Member

Choose a reason for hiding this comment

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

is that right for development?

Copy link
Member

Choose a reason for hiding this comment

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

Seems right. It gets replaced with localhost like before.

<body>
<div id='map' style='width: 100%; height: 100%;'></div>
<script>
mapboxgl.accessToken = 'pk.eyJ1IjoibWF4YW1tYW5uIiwiYSI6ImNraHlzdGNmejBkaGsycm9hNjJjcWZzdmwifQ.3NaekT5CjDsBnBNdUr1F7g';
Copy link
Member

Choose a reason for hiding this comment

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

whoopsie?

Copy link
Member

Choose a reason for hiding this comment

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

Noopsie just an alreadly public key which is not used. Dummy key.

@maxammann
Copy link
Member

Can be merged from my side.

@maxammann
Copy link
Member

I merged this now, for the rest we should create separate tasks

@maxammann maxammann merged commit 8ec9319 into main Feb 10, 2021
@maxammann maxammann deleted the 227-deb-packages branch February 10, 2021 12:37
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