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

Rahti: add certificate to store #2146

Merged
merged 3 commits into from
Apr 29, 2020
Merged

Rahti: add certificate to store #2146

merged 3 commits into from
Apr 29, 2020

Conversation

vvaltia
Copy link
Contributor

@vvaltia vvaltia commented Apr 28, 2020 •
edited
Loading

  • If certificate exists under /rems/certs/ add it to
    cacerts certificate store before starting rems

Definition of Done / Review checklist

Reviewability

  • link to issue
  • note if PR is on top of other PR
  • note if related change in rems-deploy repo
  • consider adding screenshots for ease of review

API

  • API is documented and shows up in Swagger UI
  • API is backwards compatible or completely new
  • Events are backwards compatible

Documentation

  • update changelog if necessary
  • add or update docstrings for namespaces and functions
  • components are added to guide page
  • documentation at least for config options (i.e. docs folder)
  • ADR for major architectural decisions or experiments

Different installations

  • new configuration options added to rems-deploy repository
  • instance specific translations (i.e. LBR kielivara)

Testing

  • complex logic is unit tested
  • valuable features are integration / browser / acceptance tested automatically

Accessibility

  • all icons have the aria-label attribute
  • all fields have a label
  • errors are linked to fields with aria-describedby
  • contrast is checked
  • conscious decision about where to move focus after an action

Follow-up

  • new tasks are created for pending or remaining tasks
  • no critical TODOs left to implement

@vvaltia
Rahti: add certificate to store
c225d8b 
 - If certificate exists under /rems/certs/ add it to
   cacerts certificate store before starting rems

Signed-off-by: Valtteri Valtia <[email protected]>
opqdonut
opqdonut suggested changes Apr 28, 2020
View reviewed changes
docker-entrypoint.sh
@@ -4,6 +4,19 @@ cd rems

[ -z "$COMMANDS" ] && COMMANDS="run"

certfile=$(ls /rems/certs)
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be documented somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this sufficient? https://github.com/CSCfi/rems/blob/findata-cert/docs/docker.md

@vvaltia
Add rems docker md fil:e
e8d3af6 
Signed-off-by: Valtteri Valtia <[email protected]>
opqdonut
opqdonut approved these changes Apr 29, 2020
View reviewed changes
docs/docker.md
@@ -0,0 +1,8 @@
# Deploy Rems in docker
Copy link
Contributor

Choose a reason for hiding this comment

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

could mention this file in the Dockerfile

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a mention to Dockerfile

@vvaltia vvaltia merged commit 75182c4 into master Apr 29, 2020
@vvaltia vvaltia deleted the findata-cert branch April 29, 2020 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@opqdonut opqdonut opqdonut approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@vvaltia @opqdonut