Skip to content

Remove references to convert-document#3681

Merged
stchris merged 2 commits intoalephdata:developfrom
friendly-wolfbat:develop
Apr 17, 2024
Merged

Remove references to convert-document#3681
stchris merged 2 commits intoalephdata:developfrom
friendly-wolfbat:develop

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