search

neuronets / trained-models

Trained TensorFlow models for 3D image processing
https://neuronets.dev/trained-models
22 stars 15 forks source link

feat(Workflows): Added new workflows #67

Closed gaiborjosue closed 1 year ago

gaiborjosue commented 1 year ago

This PR adds new workflows to enhance the process of adding a new model to the repository.

The workflow does the following:

For more information, please refer to the CHANGELOG.md

hvgazula commented 1 year ago

I am not sure I understand this. Why did a workflow coming from the forked repo (or PR) run here?

gaiborjosue commented 1 year ago

I am not sure I understand this. Why did a workflow coming from the forked repo (or PR) run here?

It is because github actions still evaluate changed/added workflows from the forked repo when merging.

hvgazula commented 1 year ago

Is that a default behavior? Should we not disable it?

Thanks & Regards, Harsha Gazula

On Wed, Jul 26, 2023 at 6:31 PM Edward Gaibor @.***> wrote:

I am not sure I understand this. Why did a workflow coming from the forked repo (or PR) run here?

It is because github actions still evaluate changed/added workflows from the forked repo when merging.

— Reply to this email directly, view it on GitHub https://github.com/neuronets/trained-models/pull/67#issuecomment-1652625037, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACHXDP5SQCCXYYGTQ3DFNY3XSGLCBANCNFSM6AAAAAA2ZEJU5E . You are receiving this because your review was requested.Message ID: @.***>

gaiborjosue commented 1 year ago

Is that a default behavior? Should we not disable it? Thanks & Regards, Harsha Gazula … On Wed, Jul 26, 2023 at 6:31 PM Edward Gaibor @.> wrote: I am not sure I understand this. Why did a workflow coming from the forked repo (or PR) run here? It is because github actions still evaluate changed/added workflows from the forked repo when merging. — Reply to this email directly, view it on GitHub <#67 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACHXDP5SQCCXYYGTQ3DFNY3XSGLCBANCNFSM6AAAAAA2ZEJU5E . You are receiving this because your review was requested.Message ID: @.>

I think this is just a temporal problem since after merging, that would be a default workflow. We can't disable it because it triggers on open PR.

hvgazula commented 1 year ago

Okay. But, I still find it weird that this applies to workflows in the PR in addition to those already on the branch. Feels like a security loophole for me. I don't know much about GA but I will look into it later.

hvgazula commented 1 year ago

@gaiborjosue Do you mind adding a changelog.md so we know what was done in this PR and then it will be easy for @satra to get the gist?

PS: I just converted this PR to draft but I approved all the other changes you made.

gaiborjosue commented 1 year ago

@gaiborjosue Do you mind adding a changelog.md so we know what was done in this PR and then it will be easy for @satra to get the gist?

Sure. Uploaded the changelog right now.

hvgazula commented 1 year ago

@satra I hope you will get a chance to look at this PR and let us know your comments. Thanks.

satra commented 1 year ago

overall looks good to me. i would say merge this, but this also provides the idea that instead of a PR a user could submit a model using an issue template, which in turn could create a PR. i leave this to you and harsha to merge and test.

satra commented 1 year ago

preferably do a squash and merge.

hvgazula commented 1 year ago

Thanks @satra. Good point about going from issue to PR. Will look into that.

gaiborjosue commented 1 year ago

Hello @satra & @hvgazula. I was thinking about the improvement, and the only "problem" I found is that we don't have access to the user's forked repo. Therefore, we can't do the PR from the user's branch through an issue. We would have to create a new branch in the trained model's repo with the user's files and then create a PR with that new branch. Is that ok?

hvgazula commented 1 year ago

Hello @satra & @hvgazula. I was thinking about the improvement, and the only "problem" I found is that we don't have access to the user's forked repo. Therefore, we can't do the PR from the user's branch through an issue. We would have to create a new branch in the trained model's repo with the user's files and then create a PR with that new branch. Is that ok?

You are correct. Issue to PR seems like an unnecessary complication. I think we should just leave things as they are. Let's just merge this.