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

Improve action for ligo CI #62

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

Laucans
Copy link

@Laucans Laucans commented Apr 26, 2023

  • Add possibility to override the ligo version used
  • Install ligo libraries through CI
  • Execute tests through file list
  • fix doc
  • Exit code management

@mweichert
Copy link

Hi @Laucans

Thanks so much for the PR. We appreciate the effort you've put worth here, and we really like some of the changes you proposed.

There's quite a few changes in this PR and I'd like to discuss each separately:

Typos in action.yml file

Thanks for spotting those. I've made and pushed a commit with those changes.

LIGO Libraries to install

Unfortunately, I think we need to think of how to add support for these in Taqueria itself before we can adopt an approach in the Taqueria Github Action. Taqueria already has a template mechanism, and I thought we could use that to reference LIGO templates as well. As for LIGO's package registry, I'm not sure what the best abstraction is for that yet. If you have any suggestions for how to represent these in Taqueria then I'd be happy to hear your thoughts. We'd like to hold off on providing a means for the Taqueria Github Action to reference these will we've addressed this on the Taqueria-side.

One thought is to use our template abstraction over the LIGO registry as well, but I'm not convinced that's the correct approach. What might be better is to allow LIGO packages to be installed using taq install, which would dynamically and automatically wrap a package in a plugin. I'm just not too sure how feasible that is though.

Overriding LIGO image

Taqueria provides the ability to override docker images for each plugin that uses docker under-the-hood. It looks like you've already discovered that for the LIGO, which is the TAQ_LIGO_IMAGE. Rather than expose an input called taq_ligo_image, which is specific to LIGO, I think keeping this generic to allow overriding all docker images might be a good solution. Taqueria behavior can be adjusted by specifying many different environment variables, not just ones for overriding images. As such, I think we'd rather see an input called 'env' that allows you to map many variables to values. We'd then document all environment variables that Taqueria will interpret.

However, being adding an env input, I'd like to experiment first to see if its needed - perhaps when someone uses our action in a workflow, they can already provide environment variables that will be used by the action. I'll test that and if that works, that might be our best path forward. Otherwise, what do you think of the 'env' input variable?

Exit Code Management

Great approach! I can either create a separate PR for those changes, or you can. Which would you prefer?

Sandboxes

We need to adjust the action to remove the sandbox_name input, as we already provide an environment input, and as of Taqueria 0.28.0, an environment maps to either a single sandbox, testnet, or mainnet. I see that you added a step to in the entrypoint.sh script that starts the sandbox. Can you explain why that is needed or desired rather than specifying multi-step actions:

- name: compile contracts
    uses: ecadlabs/[email protected]
    with:
        project_directory: example-projects/taco-shop
        compile_contracts: hello-tacos.mligo
        compile_plugin: smartpy

- name: start local sandbox
    uses: ecadlabs/[email protected]
    with:
        project_directory: example-projects/taco-shop
        sandbox_name: local

- name: deploy contracts
    uses: ecadlabs/[email protected]
    with:
        project_directory: example-projects/taco-shop
        deploy_contracts: hello-tacos.tz

Although the single-step action is somewhat convenient, having two different interfaces for our Github Action might a) be confusing; b) difficult to maintain.

What's your thoughts on this?

Thanks again for your contributions!

@Laucans
Copy link
Author

Laucans commented May 19, 2023

Typos in action.yml file
Thanks for spotting those. I've made and pushed a commit with those changes.

🤠

LIGO Libraries to install
Unfortunately, I think we need to think of how to add support for these in Taqueria itself before we can adopt an approach in the Taqueria Github Action. Taqueria already has a template mechanism, and I thought we could use that to reference LIGO templates as well. As for LIGO's package registry, I'm not sure what the best abstraction is for that yet. If you have any suggestions for how to represent these in Taqueria then I'd be happy to hear your thoughts. We'd like to hold off on providing a means for the Taqueria Github Action to reference these will we've addressed this on the Taqueria-side.

Agree, if ligo plugin in taqueria can manage it it's better,
Unfortunately, I need the feature on my CI right now, so this is a workaround. It can be useful while Taqueria can't manage ligo deps, but maybe you don't want to integrate it through this name :)

One thought is to use our template abstraction over the LIGO registry as well, but I'm not convinced that's the correct approach. What might be better is to allow LIGO packages to be installed using taq install, which would dynamically and automatically wrap a package in a plugin. I'm just not too sure how feasible that is though.

Overriding LIGO image

Yeah .env is the best approach if it's well documented :)

Exit Code Management
Great approach! I can either create a separate PR for those changes, or you can. Which would you prefer?

I don't think it's a great approach, it's a workaround too. A great approach should be to reuse the taq exit code but right now it's 0 in every cases.

Sandboxes

IIRC sandbox_name was mandatory for deployment. Because taq need the env to target. But TBH I don't remember this point :(
But I didn't added anything the taq sandbox start was here I just moved it (don't remember why)
image

@Laucans
Copy link
Author

Laucans commented May 19, 2023

Of course, you can cherry-pick what you want. If exit_code is a good solution for you for now, you can bring it to main without my MR :)

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.

2 participants