search

metacall / install

Cross-platform set of script to install MetaCall infrastructure.
https://metacall.io
Apache License 2.0
13 stars 14 forks source link

Trigger metacall/cli docker-hub workflow before Docker fallback tests #28

Closed SaranshBaniyal closed 1 month ago

SaranshBaniyal commented 2 months ago

This PR fixes #26 by adding a step to trigger the docker-hub.yml workflow in the metacall/cli repo before running the Docker fallback tests. This ensures the latest CLI image is updated before testing.

viferga commented 2 months ago

You do not need to force push in order to run the CI. It needs an admin to approve the CI in order to allow it to run. The best you can do is to replicate the project structure in your own profile and test it there, because this implies adding secrets to the project and we have not added them yet.

Respect to your PR, repeating the code is not the best option, you should create another step previous to those and make the other steps depend on it with the keyword needs.

SaranshBaniyal commented 2 months ago

I am sorry, I was trying to sync my fork with the latest changes and in that attempt I ended up force pushing. I will implement the needs keyword and refactor to reduce repetition, thanks for the advice. I also have one doubt, metacall/cli workflow needs to be triggered only for test-linux.yml or is there a need to implement it for test-macos.yml and 'test-windows' (since I could'nt find any docker fallback tests in those).

viferga commented 2 months ago

I am sorry, I was trying to sync my fork with the latest changes and in that attempt I ended up force pushing. I will implement the needs keyword and refactor to reduce repetition, thanks for the advice. I also have one doubt, metacall/cli workflow needs to be triggered only for test-linux.yml or is there a need to implement it for test-macos.yml and 'test-windows' (since I could'nt find any docker fallback tests in those).

Good question, for now we are only testing the docker fallback in Linux, so this should be enough, but maybe in the future we have to extend it. One good option to solve this would be to merge the three and add this step as a need for all CIs.

In other hand, try to implement this, if you cannot fork and test it on your account, push the changes with the needs command and I will enable the run of the CI here.

SaranshBaniyal commented 2 months ago

@viferga I also want to enquire that while creating a new job for metacall/cli build, I believe I should ensure that this job and the install jobs run on same docker container, how do I achieve that?

viferga commented 2 months ago

@viferga I also want to enquire that while creating a new job for metacall/cli build, I believe I should ensure that this job and the install jobs run on same docker container, how do I achieve that?

No, you don't need that. But I just realized about something, this step must be triggered only when there is a commit, never when there is a PR, because it won't work. What will happen is the following: 1) You commit some changes into master. 2) The CLI docker build is triggered, then it uses the latest install.sh to build the docker image: https://github.com/metacall/cli/blob/c6bdb48c1c55731775150353da44696c64653c3d/Dockerfile#L36 3) The image is pushed to DockerHub. 4) The CI of metacall/install is run, then it tests the docker fallback in Linux tests, by using the latest image of metacall/cli, pulled from DockerHub.

This step will be run only if we do a commit to master, not in case of PR or manual workflow dispatch.

viferga commented 2 months ago

The only thing you must do is to trigger the metacall/cli repo before the linux tests pass, only in case of commit to master.

SaranshBaniyal commented 2 months ago

I am sorry, I was trying to sync my fork with the latest changes and in that attempt I ended up force pushing. I will implement the needs keyword and refactor to reduce repetition, thanks for the advice. I also have one doubt, metacall/cli workflow needs to be triggered only for test-linux.yml or is there a need to implement it for test-macos.yml and 'test-windows' (since I could'nt find any docker fallback tests in those).

Good question, for now we are only testing the docker fallback in Linux, so this should be enough, but maybe in the future we have to extend it. One good option to solve this would be to merge the three and add this step as a need for all CIs.

In other hand, try to implement this, if you cannot fork and test it on your account, push the changes with the needs command and I will enable the run of the CI here.

Maybe we can create a new issue and a separate PR for this in future

SaranshBaniyal commented 2 months ago

@viferga I also want to enquire that while creating a new job for metacall/cli build, I believe I should ensure that this job and the install jobs run on same docker container, how do I achieve that?

No, you don't need that. But I just realized about something, this step must be triggered only when there is a commit, never when there is a PR, because it won't work. What will happen is the following:

  1. You commit some changes into master.
  2. The CLI docker build is triggered, then it uses the latest install.sh to build the docker image: https://github.com/metacall/cli/blob/c6bdb48c1c55731775150353da44696c64653c3d/Dockerfile#L36
  3. The image is pushed to DockerHub.
  4. The CI of metacall/install is run, then it tests the docker fallback in Linux tests, by using the latest image of metacall/cli, pulled from DockerHub.

This step will be run only if we do a commit to master, not in case of PR or manual workflow dispatch.

Understood, I'm on it. Will push these changes. I hope you can test them out from the main metcall account

viferga commented 1 month ago

@SaranshBaniyal the thing I told you does not work, it should test Linux during the PR and it is not doing it.. let me improve it.

viferga commented 1 month ago

@SaranshBaniyal I think it's correct now: https://github.com/metacall/install/pull/28/commits/f8ad0f464b41199fdcf5f61beae8855aeb8c6d1b