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

Gh-334: Smoother gaffer gremlin deployment #337

Merged
merged 14 commits into from
Feb 16, 2024

Conversation

tb06904
Copy link
Member

@tb06904 tb06904 commented Dec 20, 2023

Adds a new gaffer-gremlin container image that comes pre-configured to run a gremlin server with the gafferpop library using a proxy store to point at an existing Gaffer instance. Now the gaffer docs have been updated too there is a guide for users on configuring this container.

Also integrated the image build into the CI and tidied a few bits up.

Related issue

@tb06904 tb06904 linked an issue Dec 20, 2023 that may be closed by this pull request
Copy link
Member

@GCHQDeveloper314 GCHQDeveloper314 left a comment

Choose a reason for hiding this comment

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

For consistency I don't think we should be adding a new Dockerfile with a different approach to building compared to what has been done before, especially as the existing Dockerfile can simply be reused.

I would suggest keeping the existing structure and not adding the new gremlin-gaffer-demo directory, but instead create a directory under gaffer-gremlin for the demo. These can then use docker-compose for build/deployment, as done with all other images.

I'd suggest this layout

gaffer-gremlin
├── demo
│   ├── docker-compose.yaml to run the demo
│   └── other files for demo use (inc README)
├── proxy
│   ├── docker-compose.yaml to run the proxy
│   └── other files for proxy use (inc README)
├── Dockerfile
└── High level README.md

Or the proxy config could sit in the root directory for simplicity as this would be the more common usage.

@GCHQDeveloper314 GCHQDeveloper314 changed the title Gh-334 smoother gaffer gremlin deploy Gh-334: Smoother gaffer gremlin deployment Jan 9, 2024
@tb06904
Copy link
Member Author

tb06904 commented Jan 9, 2024

For consistency I don't think we should be adding a new Dockerfile with a different approach to building compared to what has been done before, especially as the existing Dockerfile can simply be reused.

I would suggest keeping the existing structure and not adding the new gremlin-gaffer-demo directory, but instead create a directory under gaffer-gremlin for the demo. These can then use docker-compose for build/deployment, as done with all other images.

I'd suggest this layout

gaffer-gremlin
├── demo
│   ├── docker-compose.yaml to run the demo
│   └── other files for demo use (inc README)
├── proxy
│   ├── docker-compose.yaml to run the proxy
│   └── other files for proxy use (inc README)
├── Dockerfile
└── High level README.md

Or the proxy config could sit in the root directory for simplicity as this would be the more common usage.

Missed this comment before committing but changes implemented pretty much are along these lines, moving into an example directory and having the proxy at the top level as its the default files in the built image (I'll add the compose file though). I'm still not sure I agree with not changing things for consistency though when there improvements that can be made, maven should really be used for dependency management of the Jars not hardcoded URLs.

@rj77259
Copy link
Member

rj77259 commented Jan 25, 2024

For consistency I don't think we should be adding a new Dockerfile with a different approach to building compared to what has been done before, especially as the existing Dockerfile can simply be reused.
I would suggest keeping the existing structure and not adding the new gremlin-gaffer-demo directory, but instead create a directory under gaffer-gremlin for the demo. These can then use docker-compose for build/deployment, as done with all other images.
I'd suggest this layout

gaffer-gremlin
├── demo
│   ├── docker-compose.yaml to run the demo
│   └── other files for demo use (inc README)
├── proxy
│   ├── docker-compose.yaml to run the proxy
│   └── other files for proxy use (inc README)
├── Dockerfile
└── High level README.md

Or the proxy config could sit in the root directory for simplicity as this would be the more common usage.

Missed this comment before committing but changes implemented pretty much are along these lines, moving into an example directory and having the proxy at the top level as its the default files in the built image (I'll add the compose file though). I'm still not sure I agree with not changing things for consistency though when there improvements that can be made, maven should really be used for dependency management of the Jars not hardcoded URLs.

I agree with tb here that we shouldn't be hardcoding URLs. If we change anything in the future we would have to change all the files which isn't best practice. Maven should do the heavy lifting. After all it is a dependency manager for a reason.

Copy link
Member

@GCHQDeveloper314 GCHQDeveloper314 left a comment

Choose a reason for hiding this comment

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

I think the build failure is unrelated to this PR. But it might be best if it is fixed before this is merged.

@GCHQDeveloper314 GCHQDeveloper314 merged commit c20dee1 into develop Feb 16, 2024
2 checks passed
@GCHQDeveloper314 GCHQDeveloper314 deleted the gh-334-smoother-gaffer-gremlin-deploy branch February 16, 2024 10:41
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.

Make Gaffer Gremlin deployment smoother
5 participants