Skip to content
This repository has been archived by the owner on Dec 16, 2019. It is now read-only.

Feature/renaming #378

Closed
wants to merge 7 commits into from
Closed

Feature/renaming #378

wants to merge 7 commits into from

Conversation

silverdaz
Copy link
Contributor

@silverdaz silverdaz commented Dec 7, 2018

Describe the pull request:

  • Bug fix
  • Functional change
  • New feature
  • Code cleanup
  • Build system change
  • Documentation change
  • Language translation

Pull request long description:

Renaming some components

Changes made:

I renamed some components.
And I added flake8 to tox dependencies (I think it was missing)
I added a dev-mode to use dummy passwords

Related issues:

Moves #380 forward

Additional information:

Release note:

Documentation change:

Mentions:

@blankdots, @viklund, @jbygdell

@silverdaz silverdaz self-assigned this Dec 7, 2018
@silverdaz silverdaz added this to the Sprint 41 milestone Dec 7, 2018
@silverdaz silverdaz force-pushed the feature/renaming branch 2 times, most recently from 594c451 to f721a82 Compare December 7, 2018 17:30
Copy link

@jbygdell jbygdell left a comment

Choose a reason for hiding this comment

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

50313cc
The path renaming docker -> deploy should go into a separate commit.

deploy/bootstrap/lega.sh Show resolved Hide resolved
deploy/test/Makefile Show resolved Hide resolved
deploy/bootstrap/lega.sh Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
deploy/bootstrap/lega.sh Show resolved Hide resolved
volumes:
- ./lega/conf.ini:/etc/ega/conf.ini:ro
restart: on-failure:3
networks:
- lega
entrypoint: ["gosu", "lega", "ega-id-mapper"]
entrypoint: ["gosu", "lega", "ega-finalize"]

# Ingestion Workers

Choose a reason for hiding this comment

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

Since you are renaming both id-mapper to finalize and s3 to vault.
You should say that in the body of the commit message, if there would have been two separate commits then the same information would have fit in then commit title.

.travis.yml Outdated
- docker-compose ps
script:
- sleep 5
- pip install -r https://raw.githubusercontent.com/NBISweden/LocalEGA-cryptor/master/requirements.txt

Choose a reason for hiding this comment

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

Remove this commit

.travis.yml Outdated
# - sudo chown -R travis private
# - docker network create cega
# - docker-compose up -d #--scale ingest=3 --scale verify=5
# - docker-compose ps

Choose a reason for hiding this comment

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

Remove this commit

@dtitov
Copy link

dtitov commented Dec 12, 2018

After force-push my comments disappear, so I'll comment here once again.

  1. Why rename docker to deploy? Yes it’s deployment, but it’s still Docker deployment. Both terms are correct, so why would we do anything about it? Just some redundant modifications, as for me. It was never explained neither here, nor in Slack, although I asked.
  2. With id-mapper -> finalize it’s up to you, of course, because it was your idea to add this component to LocalEGA. Maybe it really needs some more generic name. Maybe not. I would personally not touch it as well, but, again, I care less about this renaming.
  3. The most important here: why rename s3 to vault? Some time ago we had a separate component called Vault, it was functional module (i.e. microservice). And now it would confuse at least me, that we have Vault again, but now it’s not a microservice, but just a storage server. I would rather keep it as S3. Also Jocke mentioned that Vault can be confused with Hashicorp’s Vault for newcomers and I agree with that. And, at last, it's called S3 in Data Out, so for the sake of consistency, we need to follow common naming schemas.

@silverdaz silverdaz closed this Dec 12, 2018
@viklund viklund deleted the feature/renaming branch December 16, 2019 13:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants