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

Add possibility to build JanusGraph Scylla Release #4050

Merged

Conversation

porunov
Copy link
Member

@porunov porunov commented Oct 13, 2023

The idea in this PR is to add another profile which replaces janusgraph-cql with janusgraph-scylla when activated. Thus, allowing users to build a distribution release either with DataStax drivers or Scylla drivers.

Building distribution with CQL libs in this PR is the same as we have in our release workflow:

mvn clean install -Pjanusgraph-release -DskipTests=true --batch-mode --also-make -Dgpg.skip=true

Building distribution with Scylla libs in this PR requires activating additional profile use-scylla:

mvn clean install -Pjanusgraph-release -Puse-scylla -DskipTests=true --batch-mode --also-make -Dgpg.skip=true

Update: moved previous discussion message into the following comment, so that this PR focuses on adding possibility to build a distribution release with Scylla libraries instead of copy pasting them manually.
This PR is not about improving our current published JanusGraph distribution builds or adding support to Docker to run Scylla. Docker and published distribution builds can be improved in follow up PRs.

Related to #3580


Thank you for contributing to JanusGraph!

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

  • Is there an issue associated with this PR? Is it referenced in the commit message?
  • Does your PR body contain #xyz where xyz is the issue number you are trying to resolve?
  • Has your PR been rebased against the latest commit within the target branch (typically master)?
  • Is your initial contribution a single, squashed commit?

For code changes:

  • Have you written and/or updated unit tests to verify your changes?
  • If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
  • If applicable, have you updated the LICENSE.txt file, including the main LICENSE.txt file in the root of this repository?
  • If applicable, have you updated the NOTICE.txt file, including the main NOTICE.txt file found in the root of this repository?

For documentation related changes:

  • Have you ensured that format looks appropriate for the output in which it is rendered?

@janusgraph-bot janusgraph-bot added the cla: external Externally-managed CLA label Oct 13, 2023
Related to JanusGraph#3580

Signed-off-by: Oleksandr Porunov <alexandr.porunov@gmail.com>
@porunov porunov force-pushed the feature/scylla-and-cql-server-selection branch from 2a54cd2 to 28f237b Compare October 14, 2023 20:45
@porunov porunov marked this pull request as ready for review October 14, 2023 20:52
@porunov porunov added this to the Release v1.0.0 milestone Oct 14, 2023
@farodin91
Copy link
Contributor

It would be interesting to build both docker images with a different tag or?

@porunov
Copy link
Member Author

porunov commented Oct 14, 2023

It would be interesting to build both docker images with a different tag or?

I think we don’t necessarily need 2 docker releases (one for Scylla and another one for CQL). Instead, as I described in the referenced issue we can have an environment variable which controls if Scylla drivers or CQL drivers are used.
However, I’m OK to have a separate Docker image with ScyllaDB.
In this PR I’m just adding the ability to manually build 2 different distributions (one with DayaStax drivers and another one with Scylla drivers).

@porunov porunov requested a review from a team October 15, 2023 23:47
Copy link
Contributor

@farodin91 farodin91 left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you.

@porunov porunov merged commit 26ed54f into JanusGraph:master Oct 16, 2023
172 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/skip cla: external Externally-managed CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants