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

devops cleanups #49

Closed
wants to merge 9 commits into from
Closed

devops cleanups #49

wants to merge 9 commits into from

Conversation

astelmach01
Copy link

@astelmach01 astelmach01 commented Apr 2, 2023

  • Added pip-tools functionality. Instead of adding to requirements.txt, simply add the library to requirements.in and run pip-compile after installing pip-tools
  • Cleaned up dockerfile to install requirements first for a quick optimization
  • Changed docker image from 3.8 -> 3.10
  • Added a proper gitignore

@astelmach01 astelmach01 closed this Apr 3, 2023
@astelmach01 astelmach01 reopened this Apr 3, 2023
@astelmach01 astelmach01 changed the title added requirements.in via pip tools devops cleanups Apr 3, 2023
@ryanpeach
Copy link

Also related #137

Copy link
Contributor

@nponeccop nponeccop left a comment

Choose a reason for hiding this comment

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

Split into independent atomic PRs. While I'm not a maintainer, the current maintainers have a similar requirement in other PRs.

@@ -1,8 +1,12 @@
FROM python:3.11
FROM python:3.10-slim
Copy link
Contributor

Choose a reason for hiding this comment

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

Python bump is and using a slim image are separate design decisions. Use 2 separate PRs for this single line as it's easier to approve and merge them.


RUN pip install -r requirements.txt
COPY requirements.txt /app/requirements.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

This change is done elsewhere (in different PRs) already. Search for the PR number(s) and add a comment. Remove from this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a good idea to generate requirements.txt for a more precice dependency pindown. Explain the advantages and submit only this improvement as a separate PR.

@@ -18,7 +18,7 @@ def execute_python_file(file):
try:
client = docker.from_env()

# You can replace 'python:3.8' with the desired Python image/version
Copy link
Contributor

Choose a reason for hiding this comment

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

This typo should be a separate PR independent of all the above.

This was referenced Apr 10, 2023
@astelmach01 astelmach01 closed this by deleting the head repository Apr 14, 2023
tgonzales pushed a commit to tgonzales/Auto-GPT that referenced this pull request Apr 19, 2023
🔨 Improve local development setup to be SQLite compatible
Say383 pushed a commit to Say383/Auto-GPT that referenced this pull request Sep 8, 2023
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