Skip to content
This repository has been archived by the owner on Oct 8, 2020. It is now read-only.

Minor update for UnLynx v1.4.1 #34

Merged
merged 5 commits into from
Mar 31, 2020
Merged

Minor update for UnLynx v1.4.1 #34

merged 5 commits into from
Mar 31, 2020

Conversation

JoaoAndreSa
Copy link
Contributor

A series of commits to make medco-unlynx on par with the new unlynx v1.4.1 and onet 3.2.0:

Similar changes as https://github.com/ldsec/unlynx/releases/tag/v1.4.1

Out of topic question
For these kind of minor changes do I still do a PR to dev (and then later move to master) or should I just commit directly to dev? It's not that I'm actually developing a new feature or something :P

@mickmis
Copy link
Contributor

mickmis commented Mar 19, 2020

IMO small developments/changes like this can go directly to dev. For :

  • larger features
  • refactoring
  • something else that require more than one person development
  • something else that require review from someone else

should have its own separate branch.

We should also maybe introduce the concept of code owners in our workflow.

Whatever we agree on (including @f-marino ), this should go in the guidelines :)

Thoughts?

Copy link
Contributor

@mickmis mickmis left a comment

Choose a reason for hiding this comment

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

Looks all good, but something important should be added (and it's not valid just for your changes, but also other things we did before), all the configuration variables should be documented in e.g. the README.md.

Also the CONN_TIMEMOUT should be added into the deployment/Dockerfile + deployment/docker-compose.yml for reference.

@f-marino
Copy link

I am working to embed the comments I received on the guidelines from Linus, which actually touch also this topic. I will tell you when I am done, so that you can check it and make sure that both of you are satisfied with the final result.

@JoaoAndreSa
Copy link
Contributor Author

JoaoAndreSa commented Mar 20, 2020

Looks all good, but something important should be added (and it's not valid just for your changes, but also other things we did before), all the configuration variables should be documented in e.g. the README.md.

Also the CONN_TIMEMOUT should be added into the deployment/Dockerfile + deployment/docker-compose.yml for reference.

done 0a3b954

As for the README I am currently exporting unlynx updated documentation to gitbooks (which will include a list of env variables that you can set). For the medco-unlynx do you have any suggestions on where should I add this info, maybe medco-documentation/system-administrators/deployment/configuration?

@JoaoAndreSa
Copy link
Contributor Author

@mickmis can I merge this? I think the only thing left is adding the configuration variables to the documentation. I have them in the unlynx README, but I wonder if we should add it someplace else...

@mickmis mickmis merged commit e6769fe into dev Mar 31, 2020
@mickmis mickmis deleted the cleaning branch March 31, 2020 14:20
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.

None yet

3 participants