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

Update firefly-helm-charts to FireFly 1.3 and KiND deployment #76

Merged
merged 24 commits into from
Aug 13, 2024

Conversation

SamMayWork
Copy link
Contributor

ref: #74

This PR is only a slight change from the core of the PR above with the following differences:

  • Deploys FireFly 1.3
  • Does not directly mirror the Besu helm charts from an external provider, they're now cloned at deploy-time and applied to the cluster

@SamMayWork SamMayWork requested a review from EnriqueL8 June 17, 2024 16:04
Copy link
Contributor

@EnriqueL8 EnriqueL8 left a comment

Choose a reason for hiding this comment

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

Looking good - quite a few tweaks needed

README.md Show resolved Hide resolved
charts/firefly-evmconnect/Chart.yaml Outdated Show resolved Hide resolved
charts/firefly-signer/Chart.yaml Show resolved Hide resolved
charts/firefly-signer/Chart.yaml Outdated Show resolved Hide resolved
charts/firefly-evmconnect/Chart.yaml Outdated Show resolved Hide resolved
charts/firefly/values.yaml Outdated Show resolved Hide resolved
charts/ipfs/Chart.yaml Outdated Show resolved Hide resolved
hack/multiparty.sh Show resolved Hide resolved
values/monitoring/grafana-goquorum-dashboard.yml Outdated Show resolved Hide resolved
values/validator.yml Outdated Show resolved Hide resolved
@SamMayWork
Copy link
Contributor Author

In discussion w/ @EnriqueL8 - there's a quite a few comments from the original PR around the app versions that we're using in these charts, this mostly appears to be leftovers from the original PR, but something that we need addressing.

Problem: Right now we're not specifying a specific tag for the images that we're using for FireFly components, this means that we're defaulting to latest which might stop working at some point we don't know.

The solution we're proposing is the following:

  • This PR contains the images for FireFly 1.3.0
  • When this PR is merged, we cut a release of this repository which contains only the 1.3 images
  • A GitHub action is added which does some basic testing of the stack using KiND
  • On a regular cadence we update the images that these charts are using and then cut a new release each time

Automation to make the above easier is certainly possible and would be a meaningful contribution to this project.

@SamMayWork
Copy link
Contributor Author

SamMayWork commented Jun 24, 2024

In reference to specific comments about things in the chart that aren't in the stack we're intending to use, for example AWS/Azure/Geth...

It looks like parts of the values chart that we're using come from documentation from things like Besu and purposefully have multiple values so that they're unopinionated as possible, but not sure we want that here. Essentially, these helm charts now have 2 parts: deployment of chain infrastructure, and deployment of FireFly. For chain infrastructure we want to be very opinionated so that we don't break our KiND installation path, but our FireFly helm charts must be unopinionated.

With that in mind, I'm going to make this PR very restrictive WRT which chain infrastructure we deploy.

@SamMayWork
Copy link
Contributor Author

From comments here by @EnriqueL8 it looks like this PR being merged creates a few new items to make things here easier/more resilient:

  • Automated testing of the chart through a GitHub actions (checking that components deploy properly, with maybe some basic integration testing).
  • Hardening around the multiparty script using the status APIs to ensure that the nodes change through states as expected
  • Script to add and manage new keys into the deployed FireFly
  • Verification of the multiparty scripts against new APIs introduced into FireFly since 1.3 was released

I'll open these issues once this PR is merged

@SamMayWork
Copy link
Contributor Author

OK - Calling this PR good for a re-review, it's worth noting that currently this is not going to work because of this issue hyperledger/firefly-dataexchange-https#81, once a release has been properly cut, I'll retest this PR and we should be good for a merge.

@SamMayWork SamMayWork requested a review from EnriqueL8 June 24, 2024 10:26
@EnriqueL8
Copy link
Contributor

EnriqueL8 commented Jun 28, 2024

I've just ran this locally

W0628 12:24:56.735297   28761 warnings.go:70] unknown field "spec.automountServiceAccountToken"
Error: An error occurred while checking for chart dependencies. You may need to run `helm dependency build` to fetch missing dependencies: found in Chart.yaml, but missing in charts/ directory: firefly-evmconnect

@EnriqueL8
Copy link
Contributor

EnriqueL8 commented Jun 28, 2024

It would be nice if we could configure the swagger urls to be the port forward URL so that the swagger UI works with FireFly
image

We exposed a configuration in FireFly to allow this https://hyperledger.github.io/firefly/latest/reference/config/#http

We could also think about deploying an ingress instead of port-forwarding as it's such a common thing

Update: already available in the values.yaml would be good to automatically set that as part of the set of commands that get printed after helm upgrade

@EnriqueL8
Copy link
Contributor

Having a small tutorial on how to push to the kind registry a custom image would be amazing as well, doesn't need to be part of this PR

@EnriqueL8
Copy link
Contributor

We will hold off until 1.3.1 goes out and we will get this in!

awrichar and others added 2 commits August 5, 2024 16:39
Signed-off-by: Andrew Richardson <[email protected]>
Update versions for FireFly v1.3.1
Copy link
Contributor

@EnriqueL8 EnriqueL8 left a comment

Choose a reason for hiding this comment

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

Awesome, looks good to me now that the images have been updated

@EnriqueL8 EnriqueL8 merged commit dfa8687 into hyperledger:main Aug 13, 2024
3 checks passed
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