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

Switch from https to ffdx #156

Merged
merged 2 commits into from
Mar 23, 2022

Conversation

gabriel-indik
Copy link
Contributor

Signed-off-by: Gabriel Indik gabriel.indik@kaleido.io

Signed-off-by: Gabriel Indik <gabriel.indik@kaleido.io>
@nguyer
Copy link
Contributor

nguyer commented Mar 8, 2022

I think this also means any old config files using https instead of ffdx in existing FireFly installations will no longer work. Are we okay with that?

@gabriel-indik
Copy link
Contributor Author

@nguyer generating config files with the new agreed term shouldn't cause problems. The issue could potentially take place with hyperledger/firefly#590. The way things are today, FF is broken when ffdx is used in the configuration file. Note also that a warning is shown in the console about using https independently of what is present in the configuration file. We may need to include additional logic in FF core to properly support both scenarios.

Signed-off-by: Gabriel Indik <gabriel.indik@kaleido.io>
@onelapahead
Copy link
Contributor

Closing #154 in favor of this one

@gabriel-indik
Copy link
Contributor Author

Looking further into the code:

Current behavior

  • The CLI generates the dataexchange section in the configuration file using https and without specifying the type.
  • FF Core defaults to https when the type is missing.
  • If the dataexchange section is instead ffdx then the type must be specified, otherwise FF core will fail to read the configuration file.

Changes added to the CLI

  • The dataexchange section in the configuration file now uses ffdx instead of https
  • The type is now included (value ffdx) to ensure FF core can load the configuration file successfully

@peterbroadhurst
Copy link
Contributor

The type is now included (value ffdx) to ensure FF core can load the configuration file successfully

The fact this is necessary seem like a bug in FF Core to me.

@peterbroadhurst
Copy link
Contributor

I'm going to take a crack at a change extending hyperledger/firefly#590 that solves ^^^

Copy link
Contributor

@peterbroadhurst peterbroadhurst left a comment

Choose a reason for hiding this comment

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

Worked through the implications here, and we have an e2e run through on hyperledger/firefly#590 that confirms we still accept the https generated by the CLI before this change.

@peterbroadhurst peterbroadhurst merged commit 8f44f87 into hyperledger:main Mar 23, 2022
@peterbroadhurst peterbroadhurst deleted the switch-https-to-ffdx branch March 23, 2022 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants