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.

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

Use HTTPS by default and release v1.2.0 #44

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

slint
Copy link
Member

@slint slint commented Nov 29, 2018

No description provided.

@slint slint requested a review from lnielsen November 29, 2018 19:02
@slint
Copy link
Member Author

slint commented Nov 29, 2018

@lnielsen not sure if it should be v2.0.0 actually... One can see it as a "security fix", but on the other hand changing default parameters is breaking API...

@fenekku
Copy link
Contributor

fenekku commented Mar 14, 2023

Coordination:

slow_poke

This was put on the community PR board, so I guess I have to coordinate this 😆 ... So want to rebase this and get it in? @slint @lnielsen

@slint
Copy link
Member Author

slint commented Mar 16, 2023

I think the issue with this one is that it's a pretty big "breaking change" to switch to HTTPS URL generation, affecting many other tests e.g. on RDM REST API responses and serialized formats. This means that we'll have to first check if we have upper pins for idutils in other modules, to avoid having a wave of breaking tests.

@chokribr
Copy link
Member

Coordination: @lnielsen @slint shall we move this PR to To Review or ask @fenekku for a second reviewer aprovement to aprove!

Copy link
Contributor

@fenekku fenekku left a comment

Choose a reason for hiding this comment

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

It looks good to me.

My vote would be to make a ticket in invenio-app-rdm to

  • cap idutils in previous versions of invenio-app-rdm and related modules if not already
  • then release this
  • then integrate the new version of idutils in invenio-app-rdm and related modules

For me: better security / up-to-date schemes trumps potential backward incompatibility / small breakage. And moving sooner rather than later is better. I don't have commit/release rights on this repo anyway though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Approved 🥇
Development

Successfully merging this pull request may close these issues.

5 participants