Skip to content

Deployment Automation #137

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

Closed
wants to merge 4 commits into from
Closed

Deployment Automation #137

wants to merge 4 commits into from

Conversation

Chris9419
Copy link

Summary

This pull request adds the following deployment scripts and configurations:

  • Vagrantfile for setting up the development environment
  • Dockerfile for containerizing the application
  • docker-compose.yml for orchestrating multi-container setups
  • deploy.sh script for automating the deployment process

Why This Change Is Necessary

These changes are necessary to automate and streamline the deployment process, making it reproducible and easy to manage.

Testing Done

  • Verified that the deployment process works as expected by following the documented steps.
  • Tested the Docker containers to ensure the application runs properly with PostgreSQL.

Checklist

  • Code adheres to the project's style guidelines
  • Changes have been tested
  • Relevant documentation has been updated

@Chris9419 Chris9419 closed this Sep 4, 2024
@Chris9419 Chris9419 reopened this Sep 4, 2024
Copy link
Contributor

@snoopdave snoopdave left a comment

Choose a reason for hiding this comment

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

I don't understand the point of this PR as we already have the ability to produce a Docker file and we provide an example docker-compose.yml setup. Also, this PR introduces arbiutrary formatting changes that are not needed and make it difficult to see what has actally changed.

@@ -0,0 +1 @@
1.5:a1cf4c44-6035-470e-b080-fd4088b2dc79
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why any of the changes under .vagrant are necessary in this PR.

@@ -0,0 +1,27 @@
-----BEGIN RSA PRIVATE KEY-----
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like a bad practice to include private keys in our repo.

Copy link
Member

Choose a reason for hiding this comment

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

agreed, private keys should not be in repos many reasons.

@@ -0,0 +1,9 @@
# This file loads the proper rgloader/loader.rb file that comes packaged
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is anything related to Vagrant in this PR, we've got Docker and docker-compose already why do we need Vagrant?


WORKDIR /usr/local/tomcat
CMD [ "/usr/local/tomcat/bin/entry-point.sh" ]
FROM maven:3-openjdk-17-slim as builder
Copy link
Contributor

Choose a reason for hiding this comment

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

Formatting changes should not be included in PRs, only changes essential to the PR.

@@ -0,0 +1,99 @@
# -*- mode: ruby -*-
Copy link
Contributor

Choose a reason for hiding this comment

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

again, why do we need Vagrant when we have Docker and Docker compose?

@@ -1,47 +1,25 @@

Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this PR remove the license header? Also, any new files shiould include the license header.

- "5432:5432"
volumes:
- type: bind
source: C:/roller/docker/postgresql-data
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there no way to specify a Docker file like this without OS specific paths like this?

Copy link
Member

@mbien mbien left a comment

Choose a reason for hiding this comment

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

unless there is a committer who is willing to look over this and maintain the vagrant config, i am going to -1 this.

The main artifacts of this repo are the zip/war. Everything what comes after this can be customized by the user who deploys roller. There is also a docker file serving as example deployment / getting started script.

You could of course host a sample vagrant config in your own repo, but I don't see the purpose of this in the main repo right now esp since we would have to maintain this forever.

repos should also not contain private keys, system specific paths etc but dave already annotated those bits.

@@ -0,0 +1,27 @@
-----BEGIN RSA PRIVATE KEY-----
Copy link
Member

Choose a reason for hiding this comment

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

agreed, private keys should not be in repos many reasons.

@snoopdave snoopdave closed this Sep 21, 2024
@snoopdave
Copy link
Contributor

I don't think this is work that we want to merge into Roller.

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