reanahub / reana

REANA: Reusable research data analysis platform
https://docs.reana.io
MIT License
127 stars 54 forks source link

Merge `maint-0.7` to `master` #480

Closed mvidalgarcia closed 3 years ago

mvidalgarcia commented 3 years ago

Recipe:

$ reana-dev git-upgrade -c ALL
$ reana-dev git-checkout -c ALL master
$ cd reana-ui
$ reana-dev git-upgrade -c . --base maint-0.7
$ reana-dev git-upgrade -c .
$ git checkout -b merge
$ git log master..maint-0.7  # what changes we would merge?
$ git merge --log maint-0.7  
$ git status # file1.py and file2.py have conflicts
$ vim file1.py # solve conflicts
$ git add file1.py
$ vim file2.py # solve more conflicts
$ git add file2.py
$ git status # all clean
$ git merge --continue # we are done!
$ git diff master merge # inspect all code changes
$ reana-dev run-ci ... # try to run; whoops, it's broken
$ vim file3.py # some other file may needs adaptation
$ git add file3.py
$ git commit --amend
$ git show master:file4.py > file4.py # some yet another file may be easier to edit out again from its master state
$ vim file4.py
$ git add file4.py
$ git commit --amend
$ git diff master merge # one last look
$ reana-dev run-ci ... # let's retest 
TODO:
mvidalgarcia commented 3 years ago

TODO:

  • Test user email confirmation features
  • Test hiding sign up form

Tested with the following status and working as expected:

reana-workflow-controller @ merge @ 24c3a62 Merge branch 'maint-0.7'
reana-workflow-engine-serial @ merge @ b9219f6 Merge branch 'maint-0.7'
reana-client @ merge @ 03460b7 Merge branch 'maint-0.7'
reana-workflow-engine-cwl @ merge @ b21a2e6 Merge branch 'maint-0.7'
reana-commons @ merge @ 0ff3eb9 Merge branch 'maint-0.7'
reana @ merge @ bb3be04 Merge branch 'maint-0.7'
reana-db @ merge @ a3adc68 Merge branch 'maint-0.7'
reana-message-broker @ merge @ 5704409 Merge branch 'maint-0.7'
reana-workflow-engine-yadage @ merge @ e20432b Merge branch 'maint-0.7'
pytest-reana @ master @ e191c31 release: 0.8.0a3
reana-server @ merge @ b074a23 Merge branch 'maint-0.7'
reana-ui @ merge @ ec6ced8 Merge branch 'maint-0.7'
reana-job-controller @ merge @ 85db1af Merge branch 'maint-0.7'

The only detail that was a little bit confusing is the fact that, since notifications are disabled by default, and now we have the REANA_USER_EMAIL_CONFIRMATION enabled by default, it fails when it tries to send the confirmation email. This is because the send_email() function now raises an exception instead of failing silently as it used to be (which is definitely useful to catch issues on PROD). In addition, even when REANA_USER_EMAIL_CONFIRMATION is disabled, an exception occurs when requesting/granting/revoking the token as it involves email sending.

Summing up, this means that the default values.yaml configuration we currently have, fails when untouched in when some actions are performed, which I guess it's not ideal.

At first sight I see two options to solve this:

tiborsimko commented 3 years ago

The PR set works well :+1: Going to proceed to checking and merging PRs one by one.

WRT A vs B, this will apply also to maint-0.7 branch, so we can solve the issue there. Currently, we simply warn admins via docs that they should set up sender information, but also SMTP information, for the email notification service to work. I guess we could make a nicer check on the application level, e.g. you can get into a situation where SMTP credential is outdated... So runtime treatment to make these "continuable error" situations would be preferable. We can create a new issue on this for maint-0.7.