Skip to content

Conversation

@gabriel-indik
Copy link
Contributor

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

@gabriel-indik
Copy link
Contributor Author

Important: hyperledger/firefly-cli#156 should be merged before this change.

@codecov-commenter
Copy link

Codecov Report

Merging #590 (8772cc6) into main (00fda51) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #590   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          303       303           
  Lines        17437     17437           
=========================================
  Hits         17437     17437           
Impacted Files Coverage Δ
internal/config/config.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 00fda51...8772cc6. Read the comment docs.

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.

The fact the E2E is failing without hyperledger/firefly-cli#156 merging might mean we need a tweak to the migration code in FireFly:

func InitPrefix(prefix config.Prefix) {
for name, plugin := range pluginsByName {
plugin().InitPrefix(prefix.SubPrefix(name))
// Migration path for old plugin name
// TODO: remove this
if name == "ffdx" {
plugin().InitPrefix(prefix.SubPrefix("https"))
}
}
}

@gabriel-indik
Copy link
Contributor Author

Closing based on the observations in hyperledger/firefly-cli#156 (comment)

gabriel-indik and others added 2 commits March 22, 2022 17:12
Signed-off-by: Gabriel Indik <gabriel.indik@kaleido.io>
Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
@peterbroadhurst
Copy link
Contributor

I do believe we need to fix this default @gabriel-indik, so I've done some work to fix the migration to:

  1. Cover the case of https.url being set, with ffdx.url not being set
  2. Avoid initializing the prefix on the default paths, so that things like ff config do not show the outdated prefix

@peterbroadhurst peterbroadhurst dismissed their stale review March 23, 2022 01:40

deferring to @nguyer for review

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
@nguyer nguyer merged commit 1c0672c into hyperledger:main Mar 23, 2022
@nguyer nguyer deleted the switch-https-to-ffdx branch March 23, 2022 15:06
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