Skip to content

Comments

Use go:embed instead of packr#5529

Merged
makramkd merged 15 commits intodevelopfrom
chore/sc-21770
Nov 29, 2021
Merged

Use go:embed instead of packr#5529
makramkd merged 15 commits intodevelopfrom
chore/sc-21770

Conversation

@makramkd
Copy link
Contributor

@makramkd makramkd commented Nov 26, 2021

Replace packr with Go 1.16's embed feature.

Here's a summary of the changes:

  1. Use go:embed instead of packr.Box to embed the build assets of operator-ui into the chainlink binary.
  2. In order to be able to use go:embed, we must have the assets available in the same folder as the module in which the go:embed directive is supplied. In this case, the assets must be available in core/web. Therefore, the webpack configuration was changed to output the compiled static assets (the HTML, JavaScript, and SVG files) in core/web instead of operator_ui/dist.
  3. As a consequence of doing (2), we need to update the chainlink dockerfile.

@github-actions
Copy link
Contributor

I see that you haven't updated any CHANGELOG files. Would it make sense to do so?

github.com/go-playground/validator/v10 v10.4.1 // indirect
github.com/go-stack/stack v1.8.0 // indirect
github.com/gobuffalo/envy v1.7.0 // indirect
github.com/gobuffalo/logger v1.0.0 // indirect
Copy link
Contributor

@archseer archseer Nov 26, 2021

Choose a reason for hiding this comment

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

Don't forget to remove it from the dependency list at the top of go.mod too so we stop downloading it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

Copy link
Contributor

Choose a reason for hiding this comment

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

You probably also need a go mod tidy now? There's still some references in go.sum

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is odd, I still see packr in the go.sum even after I run a go mod tidy. Even after manually deleting those entries in go.sum and re-calling go mod tidy it still adds them back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, another thing I noticed is that go mod tidy only removes some versions of a dependency from go.sum and not others. Not sure why that is. E.g godirwalk, godotenv, go-internal, and logrus still have entries in go.sum.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So despite this being somewhat annoying, I think this is alright. The go.sum file basically just has hashes of modules at certain versions, and it does this to verify dependencies it downloaded (as a security feature). It doesn't mean that the dep will be downloaded though, so it's not dependency bloat. The go.mod file however is where deps are declared and that's the source of truth on what modules are required by the app.

@jkongie
Copy link
Contributor

jkongie commented Nov 26, 2021 via email

@makramkd makramkd marked this pull request as ready for review November 26, 2021 21:41
archseer
archseer previously approved these changes Nov 27, 2021
@archseer
Copy link
Contributor

Oh, since the contracts are no longer embedded, I wonder if we could move

- name: Compile all contracts
run: ./contracts/scripts/native_solc_compile_all
- name: Verify local solc binaries
run: ./tools/ci/check_solc_hashes
under the solidity workflow.

samsondav
samsondav previously approved these changes Nov 29, 2021
Copy link
Contributor

@samsondav samsondav left a comment

Choose a reason for hiding this comment

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

Nice work

@makramkd makramkd dismissed stale reviews from samsondav and archseer via c67565a November 29, 2021 14:06
Comment on lines -64 to -67
- name: Compile all contracts
run: ./contracts/scripts/native_solc_compile_all
- name: Verify local solc binaries
run: ./tools/ci/check_solc_hashes
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we still do this in the Solidity workflow? It currently only runs the tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can move this there, since we don't do it there.

Copy link
Contributor

@archseer archseer left a comment

Choose a reason for hiding this comment

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

Great work! 🎉

@makramkd makramkd merged commit c993872 into develop Nov 29, 2021
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