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

Remove references to convert-document #3681

Merged
merged 2 commits into from
Apr 17, 2024
Merged

Remove references to convert-document #3681

merged 2 commits into from
Apr 17, 2024

Conversation

friendly-wolfbat
Copy link
Contributor

Per #3680 (comment), I am attempting a pull request to remove references to the old convert-document container, which has since been consolidated into ingest-file (see #2755).

This is my first pull request and I am open to learning! Please let me know if anything is wrong or if I need to make any adjustments.

Copy link
Contributor

@stchris stchris left a comment

Choose a reason for hiding this comment

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

I left a few remarks, thanks for cleaning this up!

@friendly-wolfbat
Copy link
Contributor Author

Thanks so much for reviewing. I believe I made all the changes requested.

A couple of unrelated things I noticed:

  1. Looks like the documentation is referring to docker compose as docker-compose. The latter is for version 1 of Docker Compose, which has since been deprecated. Should I make a separate pull request to remove the dash from the docker compose commands?
  2. In the installation.mdx file, there are references to aleph-worker containers; they are also referred to as worker. Should they be consistent, or are they different? Which should the documentation be using?

@stchris
Copy link
Contributor

stchris commented Apr 17, 2024

Thanks for your changes, I will review them shortly!

1. Looks like the documentation is referring to `docker compose` as `docker-compose`. The latter is for version 1 of Docker Compose, which has since been [deprecated](https://www.docker.com/blog/new-docker-compose-v2-and-v1-deprecation/). Should I make a separate pull request to remove the dash from the `docker compose` commands?

Ah yes, good observation. I believe for consistency we should do this in a separate PR across all of our documentation.

2. In the installation.mdx file, there are references to `aleph-worker` containers; they are also referred to as `worker`. Should they be consistent, or are they different? Which should the documentation be using?

Another good observation. So if one uses our Helm chart everything would be prefixed with aleph-, but since we are not referring to api or ingest-file containers with that prefix we should be consistent here as well and call it worker. And that should ideally be a PR of its own as well.

@stchris stchris merged commit 04c39de into alephdata:develop Apr 17, 2024
@stchris
Copy link
Contributor

stchris commented Apr 17, 2024

Thanks for your contribution, @friendly-wolfbat

@friendly-wolfbat
Copy link
Contributor Author

@stchris Hey, I'm sorry, it looks like the hyperlink I had to the installation page is wrong, when looking at the "Checks" tab. Should I have put a slash before the hashtag?

@stchris
Copy link
Contributor

stchris commented Apr 22, 2024

Don't worry about it, @friendly-wolfbat ! It is our fault because there need to be some CI checks on PRs which are currently not in place. I will take care of fixing it.

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.

2 participants