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.

Sign up for GitHub

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

Jump to bottom

#1363, #1347: Make docker-compose the preferred way to try cartography #1364

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

#1363, #1347: Make docker-compose the preferred way to try cartography #1364

wants to merge 6 commits into from
+170 −162

Conversation

achantavy
Copy link
Contributor

@achantavy achantavy commented Oct 2, 2024 •
edited
Loading

Summary

Describe your changes.

Addresses #1363 and #1347.

  • Removes Dockerfile because it was duplicative of dist.Dockerfile, didn't correctly handle pre-commit after .git/ was removed from the image builds due to security reasons, and it confused users.
  • Promotes dist.Dockerfile to Dockerfile. This is intended to distribute cartography with its Python dependencies.
  • Updates all relevant documentation to prefer running docker-compose for end-users. Cartography developers still need a native Python installation.

Checklist

Provide proof that this works (this makes reviews move faster). Please perform one or more of the following:

  • Update/add unit or integration tests.
  • Include a screenshot showing what the graph looked like before and after your changes.
  • Include console log trace showing what happened before and after your changes.

Proof that this works

  • Building the image
    quirkydockererror

  • Running an AWS sync using docker-compose

docker-compose-run

CLI used:

docker-compose run -e AWS_PROFILE=1234_testprofile -e AWS_DEFAULT_REGION=us-east-1 cartography --neo4j-uri bolt://cartography-neo4j-1:7687

@achantavy achantavy linked an issue Oct 2, 2024 that may be closed by this pull request
Dockerfile --local can only be used inside a git repository #1363
Open
@achantavy achantavy removed the request for review from chandanchowdhury October 2, 2024 06:59
@achantavy
Copy link
Contributor Author

This still needs more work, Docker is weird.

@achantavy achantavy changed the title #1363, #1347: Fix Dockerfile and docker-compose [WIP] #1363, #1347: Fix Dockerfile and docker-compose Oct 2, 2024
@achantavy achantavy changed the title [WIP] #1363, #1347: Fix Dockerfile and docker-compose #1363, #1347: Fix Dockerfile and docker-compose Oct 3, 2024
@achantavy achantavy changed the title #1363, #1347: Fix Dockerfile and docker-compose #1363, #1347: Make docker-compose the preferred way to try cartography Oct 3, 2024
@achantavy
Copy link
Contributor Author

cc: @hatchetation -- I think this addresses the doc bugs you brought up. I think it makes sense to have docker-compose be the main way to try out cartography.

chandanchowdhury
chandanchowdhury requested changes Oct 3, 2024
View reviewed changes
Dockerfile
DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends python3.10-dev python3-pip python3-setuptools openssl libssl-dev gcc pkg-config libffi-dev libxml2-dev libxmlsec1-dev curl make git && \
apt-get clean && \
rm -rf /var/lib/apt/lists/*
COPY . /var/cartography
Copy link
Collaborator

Choose a reason for hiding this comment

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

For Production docker image, we must install Cartography from PyPI.

Dockerfile

# Installs pip supported by python3.10
RUN curl https://bootstrap.pypa.io/get-pip.py -o get-pip.py && python3.10 get-pip.py
RUN pip install -U -e .
Copy link
Collaborator

Choose a reason for hiding this comment

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

Production install should not use --editable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the way it already is right now though: https://github.com/lyft/cartography/blob/master/dist.Dockerfile

The other one that I removed is this way too:

cartography/Dockerfile

Line 20 in 6a6e277

RUN pip install -e . && \

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chandanchowdhury chandanchowdhury chandanchowdhury requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Dockerfile --local can only be used inside a git repository
2 participants
@achantavy @chandanchowdhury