You are viewing a plain text version of this content. The canonical link for it is here.

Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/01/06 11:56:36 UTC

[GitHub] [airflow] potiuk opened a new issue #13513: Switch external actions to subrepo'ed .github/actions/

potiuk opened a new issue #13513:
URL: https://github.com/apache/airflow/issues/13513

Due to security changes imposed by INFRA, we had to switch our GitHub actions to use "apache/aiflow-" repositories.
Discussion here: https://lists.apache.org/thread.html/rfc3269829edc672f928d8304d7b0aa38d1d2a5f5cb177af95e98a1e5%40%3Cbuilds.apache.org%3E

This was a quick reaction to the issue, but seems we can do better. Following the way Apache Beam has done it (https://github.com/apache/beam/pull/13670/files) we can do similarly. We can bring in the external actions as subdirectories of the .github/actions folders. The only small modification I'd use is to use `subrepo` to add the actions.

This approach is great when it comes to reviewing the code and any changes incoming (because they go through regular review process and all the code is in). Also it is really nice approach if you want to quickly modify the action - you can do it all as part of the PR where you test it.

It has a drawback of slightly more complex case when you want to upgrade to a newer version of the action, but it can be easily handled if you use subrepo: https://github.com/ingydotnet/git-subrepo.

We've used subrepo extensively in a few projects and I really like it. It allows to turn linked repositories (and changes incoming) into single 'squashed' commits that land as 'subdirectory" in you repository. And the subrepo records the source of the 'subrepo' (location and commit), so if you want to update to another version, it will automatically get only the commit that changed since your previous pull.

The code is committed together with your code (so as opposed to submodules you do not have to pull the repo separately). What's even more, when you make local changes, it will also allow you to make PR out of that to include it in the upstream code. So you can easily make your local modification and subrepo will keep track of that and allows also to merge it with the incoming changes if you want to upgrade to newer version.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk edited a comment on issue #13513: Switch external actions to subrepo'ed .github/actions/

Posted by GitBox <gi...@apache.org>.

potiuk edited a comment on issue #13513:
URL: https://github.com/apache/airflow/issues/13513#issuecomment-755269654






----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] kaxil commented on issue #13513: Switch external actions to subrepo'ed/submodules .github/actions/

Posted by GitBox <gi...@apache.org>.

kaxil commented on issue #13513:
URL: https://github.com/apache/airflow/issues/13513#issuecomment-755830322


   +1 with submodules -- looks clean (having seen your example PR on astro repo)


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk commented on issue #13513: Switch external actions to subrepo'ed .github/actions/

Posted by GitBox <gi...@apache.org>.

potiuk commented on issue #13513:
URL: https://github.com/apache/airflow/issues/13513#issuecomment-755269654


   > The main reason I'd prefer submodules over subrepo here is number of commits: having all the commit history for all the actions in our repo is "noisy", and coupuled with this only being used on CI everyone can basically ignore the fact we are using submodules -- development will happily work fine without them existing.
   
   That's not the case: when we bring a repo from outside using subrepo we can choose "squash" method as the default. It means that we will have ONE squashed  commit to bring it in and ONE squashed commit every time we bring a new version (regardless how many commits were there in the original repo). Subrepo handles it beautifully for us.
   
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk edited a comment on issue #13513: Switch external actions to subrepo'ed .github/actions/

Posted by GitBox <gi...@apache.org>.

potiuk edited a comment on issue #13513:
URL: https://github.com/apache/airflow/issues/13513#issuecomment-755261879

CC: @kaxil @ashb @turbaszek @TobKed -> WDYT ? I think this is much better approach than the cloned repositories we have now.

Maybe @TobKed you would like to take a stab on it ? Happy to help as I've done it several times and know subrepo ins-outs very well.

Also anticipating comments about "yet another tool" - > subrepo is great because it does not require ANY action from the users of the repositories that bring in the subrepositories.

Unlike submodule, it does not require any change of the worklfow (when you checkout the repo the code for the repo just appears there. From the repository point of view, this is just a single, usually squashed commit where you commit the subrepo, with all the new code included, plus a single file (.subrepo I think) where subrepo records the URL of the original repo + commit hash/branch it was copied from. The only worlflows that require subrepo are:

a) when you want to update to newer version of the repo
b) when you want to upstream local change

And we worked out and documented that workflows nicely in one of the projects we worked on so we can bring the "step-by-step" instructions easily.

The nicest thing about this approach is that:

a) You see the code of the action in your repo, so you can easier find out what's wrong if there are some problems
b) You can easily make modification to those actions in your PR and they can be tested in much simpler workflow (just modify code in your PR as opposed to:
1. modify action
2. hope that it works
3. change hash commit
4. update PR

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk commented on issue #13513: Switch external actions to subrepo'ed .github/actions/

Posted by GitBox <gi...@apache.org>.

potiuk commented on issue #13513:
URL: https://github.com/apache/airflow/issues/13513#issuecomment-755323214


   > Yeah, that new submodule review view is new sometime in the last few years -- makes them much easier to use.
   > 
   > Note: I an still generally against submodules as they are difficult to use, but since this is _only_ for CI and everyone* else is can checkout the repo and not have to care that the submodules exist or not I think it the right tool for this use case.
   
   Yeah. I also do not like submodules for anything "serious" but I think - seeing how Github handles the review - it passes all my requirements I had and it is far 'lighter' on our repo than subrepo, so if it works. I am all for it.
   
   
   
   > so I don't even see where we need to leave comments on the actions code
   
   I thought about comments like *Hey do you think we have a security problem. WDYT @ashb" :). But seems submodule + GitHub handle that in a very good way, so no problem with submodules in this case.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk edited a comment on issue #13513: Switch external actions to subrepo'ed/submodules .github/actions/

Posted by GitBox <gi...@apache.org>.

potiuk edited a comment on issue #13513:
URL: https://github.com/apache/airflow/issues/13513#issuecomment-755324649


   > I thought about comments like *Hey do you think we have a security problem. WDYT @ashb" :). But seems submodule + GitHub handle that in a very good way, so no problem with submodules in this case.
   
   The only (small) problem I have is in case we want to actually change and modify the action, it is far easier in subrepo (changes in multiple repositories that need to be pushed in-sync), But it's something that happens rarely enough to not be a real issue.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] ashb commented on issue #13513: Switch external actions to subrepo'ed .github/actions/

Posted by GitBox <gi...@apache.org>.

ashb commented on issue #13513:
URL: https://github.com/apache/airflow/issues/13513#issuecomment-755320256


   Yeah, that new submodule review view is new sometime in the last few years -- makes them much easier to use.
   
   Note: I an still generally against submodules as they are difficult to use, but since this is _only_ for CI and everyone* else is can checkout the repo and not have to care that the submodules exist or not I think it the right tool for this use case.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk closed issue #13513: Switch external actions to subrepo'ed/submodules .github/actions/

Posted by GitBox <gi...@apache.org>.

potiuk closed issue #13513:
URL: https://github.com/apache/airflow/issues/13513


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk commented on issue #13513: Switch external actions to subrepo'ed/submodules .github/actions/

Posted by GitBox <gi...@apache.org>.

potiuk commented on issue #13513:
URL: https://github.com/apache/airflow/issues/13513#issuecomment-755324649


   > I thought about comments like *Hey do you think we have a security problem. WDYT @ashb" :). But seems submodule + GitHub handle that in a very good way, so no problem with submodules in this case.
   
   The only (small) problem I have is in case we want to actually change and modify the action, it is far easier in subrepo (changes in multiple repositories that need to be push in-sync), But it's something that happens rarely enough to not be a real issue.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk edited a comment on issue #13513: Switch external actions to subrepo'ed .github/actions/

Posted by GitBox <gi...@apache.org>.

potiuk edited a comment on issue #13513:
URL: https://github.com/apache/airflow/issues/13513#issuecomment-755268345


   > Reading https://github.community/t/github-action-defined-as-submodule-in-a-repo/139204/12 it looks like we can use git submodules for actions so long as ASF have allowed us to use official actions, which I think they have.
   > 
   > Thoughts on submodules here @potiuk ?
   
   I am not sure if it would work - especially parsing action inputs and verifying them at scheduler.  But regardless of the capabilities, I believe this would be strictly agains the problem INFRA tries to avold. The idea is to make sure action code is reviewed before it is used and we are not supposed to use code unreviewed by someone at apache.
   
   The subrepo approach follows exactly this recommentdation that the code (and any change to the actions) MUST be reviewed before it gets used in the master 
   
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk edited a comment on issue #13513: Switch external actions to subrepo'ed .github/actions/

Posted by GitBox <gi...@apache.org>.

potiuk edited a comment on issue #13513:
URL: https://github.com/apache/airflow/issues/13513#issuecomment-755261879

CC: @kaxil @ashb @turbaszek @TobKed -> WDYT ? I think this is much better approach than the cloned repositories we have now.

Maybe @TobKed you would like to take a stab on it ? Happy to help as I've done it several times and know subrepo ins-outs very well.

Also anticipating comments about "yet another tool" - > subrepo is great because it does not require ANY action from the users of the repositories that are brought in. Unlike submodule, it does not require any change of the worklfow (when you checkout the repo the code for the repo just appears there. From the repository point of view, this is just a single, usually squashed commit where you commit the subrepo, with all the new code included, plus a single file (.subrepo I think) where subrepo records the URL of the original repo + commit hash/branch it was copied from. The only worlflows that require subrepo are:

a) when you want to update to newer version of the repo
b) when you want to upstream local change

And we worked out and documented that workflows nicely in one of the projects we worked on so we can bring the "step-by-step" instructions easily.

The nicest thing about this approach is that:

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk commented on issue #13513: Switch external actions to subrepo'ed .github/actions/

Posted by GitBox <gi...@apache.org>.

potiuk commented on issue #13513:
URL: https://github.com/apache/airflow/issues/13513#issuecomment-755268345


   > Reading https://github.community/t/github-action-defined-as-submodule-in-a-repo/139204/12 it looks like we can use git submodules for actions so long as ASF have allowed us to use official actions, which I think they have.
   > 
   > Thoughts on submodules here @potiuk ?
   
   I am not sure if it would work - especially parsing action parameters.  But regardless of the capabilities, I believe this would be strictly agains the problem INFRA tries to avold. The idea is to make sure action code is reviewed before it is used and we are not supposed to use code unreviewed by someone at apache.
   
   The subrepo approach follows exactly this recommentdation that the code (and any change to the actions) MUST be reviewed before it gets used in the master 
   
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk edited a comment on issue #13513: Switch external actions to subrepo'ed .github/actions/

Posted by GitBox <gi...@apache.org>.

potiuk edited a comment on issue #13513:
URL: https://github.com/apache/airflow/issues/13513#issuecomment-755261879

CC: @kaxil @ashb @turbaszek @TobKed -> WDYT ? I think this is much better approach than the cloned repositories we have now.

Maybe @TobKed you would like to take a stab on it ? Happy to help as I've done it several times and know subrepo ins-outs very well.

Also anticipating comments about "yet another tool" - > subrepo is great because it does not require ANY action from the users of the repositories that bring in the subrepositories.

Unlike submodule, it does not require any change of the worklfow (when you checkout the repo the code for the repo just appears there). From the repository point of view, this is just a single, usually squashed commit where you commit the subrepo, with all the new code included, plus a single file (.subrepo I think) where subrepo records the URL of the original repo + commit hash/branch it was copied from. The only worlflows that require subrepo are:

a) when you want to update to newer version of the repo
b) when you want to upstream local change

And we worked out and documented that workflows nicely in one of the projects we worked on so we can bring the "step-by-step" instructions easily.

The nicest thing about this approach is that:

a) You see the code of the action in your repo, so you can easier find out what's wrong if there are some problems
b) You can easily make modification to those actions in your PR and they can be tested in much simpler workflow. You just modify code in your PR as opposed to:
1. modify action
2. hope that it works
3. change hash commit
4. update PR

And the nicest thing is that subrepo nicely allows to keep track of the changes.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk edited a comment on issue #13513: Switch external actions to subrepo'ed .github/actions/

Posted by GitBox <gi...@apache.org>.

potiuk edited a comment on issue #13513:
URL: https://github.com/apache/airflow/issues/13513#issuecomment-755269654


   > The main reason I'd prefer submodules over subrepo here is number of commits: having all the commit history for all the actions in our repo is "noisy", and coupuled with this only being used on CI everyone can basically ignore the fact we are using submodules -- development will happily work fine without them existing.
   
   That's not the case: when we bring a repo from outside using subrepo we can choose "squash" method as the default. It means that we will have ONE squashed  commit to bring it in and ONE squashed commit every time we bring a new version (regardless how many commits were there between those two in the original repo). Subrepo handles it beautifully for us.
   
   
   > So I guess I do have an opinion: no to subrepo or anything else that brings the commits for the actions in to our repo and "pollutes" our git history.
   
   So effectively you have the same number of commits (albeit the commits are bigger) when you use subrepo and submodule. Every time there is a new version brought in, you have one commit.
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] ashb commented on issue #13513: Switch external actions to subrepo'ed .github/actions/

Posted by GitBox <gi...@apache.org>.

ashb commented on issue #13513:
URL: https://github.com/apache/airflow/issues/13513#issuecomment-755264195


   Reading https://github.community/t/github-action-defined-as-submodule-in-a-repo/139204/12 it looks like we can use git submodules for actions so long as ASF have allowed us to use official actions, which I think they have.
   
   Thoughts on submodules here @potiuk ?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk edited a comment on issue #13513: Switch external actions to subrepo'ed .github/actions/

Posted by GitBox <gi...@apache.org>.

potiuk edited a comment on issue #13513:
URL: https://github.com/apache/airflow/issues/13513#issuecomment-755269654


   > The main reason I'd prefer submodules over subrepo here is number of commits: having all the commit history for all the actions in our repo is "noisy", and coupuled with this only being used on CI everyone can basically ignore the fact we are using submodules -- development will happily work fine without them existing.
   
   That's not the case: when we bring a repo from outside using subrepo we can choose "squash" method as the default. It means that we will have ONE squashed  commit to bring it in and ONE squashed commit every time we bring a new version (regardless how many commits were there between those two in the original repo). Subrepo handles it beautifully for us.
   
   So effectively you have the same number of commits (albeit the commits are bigger) when you use subrepo and submodule. Every time there is a new version brought in, you have one commit.
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk edited a comment on issue #13513: Switch external actions to subrepo'ed .github/actions/

Posted by GitBox <gi...@apache.org>.

potiuk edited a comment on issue #13513:
URL: https://github.com/apache/airflow/issues/13513#issuecomment-755261879

CC: @kaxil @ashb @turbaszek @TobKed -> WDYT ? I think this is much better approach than the cloned repositories we have now.

Maybe @TobKed you would like to take a stab on it ? Happy to help as I've done it several times and know subrepo ins-outs very well.

a) when you want to update to newer version of the repo
b) when you want to upstream local change

And we worked out and documented that workflows nicely in one of the projects we worked on so we can bring the "step-by-step" instructions easily.

The nicest thing about this approach is that:

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] ashb edited a comment on issue #13513: Switch external actions to subrepo'ed .github/actions/

Posted by GitBox <gi...@apache.org>.

ashb edited a comment on issue #13513:
URL: https://github.com/apache/airflow/issues/13513#issuecomment-755264195


   Reading https://github.community/t/github-action-defined-as-submodule-in-a-repo/139204/12 it looks like we can use git submodules for actions so long as ASF have allowed us to use official actions, which I think they have.
   
   Thoughts on submodules here @potiuk?
   
   ```yaml
       steps:
         - name: Checkout repository
           uses: actions/checkout@v2
           with:
             submodules: recursive
         - name: Use submodule action
           uses: ./.github/actions/checkout
           with:
             ...
   ```
   
   The main reason I'd prefer submodules over subrepo here is number of commits: having all the commit history for all the actions in our repo is "noisy", and coupuled with this only being used on CI everyone can basically ignore the fact we are using submodules -- development will happily work fine without them existing.
   
   So I guess I do have an opinion: no to subrepo or anything else that brings the commits for the actions in to our repo and "pollutes" our git history.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk edited a comment on issue #13513: Switch external actions to subrepo'ed .github/actions/

Posted by GitBox <gi...@apache.org>.

potiuk edited a comment on issue #13513:
URL: https://github.com/apache/airflow/issues/13513#issuecomment-755303986


   > Submodules or subrepo almost the same reviewability - it still requires a PR to be merged and approved, and it is pinned to a specific commit.
   
   Not really - you have to specifically go to the other repo in the right version and review it. That a lot more effort, you cannot leave comments etc. . With subrepo, the changed code is part of your PR and you review it together with other code. This makes perfect sense and it's much more natural.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk edited a comment on issue #13513: Switch external actions to subrepo'ed .github/actions/

Posted by GitBox <gi...@apache.org>.

potiuk edited a comment on issue #13513:
URL: https://github.com/apache/airflow/issues/13513#issuecomment-755261879

CC: @kaxil @ashb @turbaszek @TobKed -> WDYT ? I think this is much better approach than the cloned repositories we have now.

Maybe @TobKed you would like to take a stab on it ? Happy to help as I've done it several times and know subrepo ins-outs very well.

Also anticipating comments about "yet another tool" - > subrepo is great because it does not require ANY action from the users of the repositories that bring in the subrepositories.

a) when you want to update to newer version of the repo
b) when you want to upstream local change

And we worked out and documented that workflows nicely in one of the projects we worked on so we can bring the "step-by-step" instructions easily.

The nicest thing about this approach is that:

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] ashb commented on issue #13513: Switch external actions to subrepo'ed/submodules .github/actions/

Posted by GitBox <gi...@apache.org>.

ashb commented on issue #13513:
URL: https://github.com/apache/airflow/issues/13513#issuecomment-755324497


   Once/if https://github.com/apache/airflow/pull/13514 passes (I think it should) I'll convert all the rest of the third party actions over to this pattern.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk commented on issue #13513: Switch external actions to subrepo'ed .github/actions/

Posted by GitBox <gi...@apache.org>.

potiuk commented on issue #13513:
URL: https://github.com/apache/airflow/issues/13513#issuecomment-755314945


   > 
   > Github handles it nicer than that -- checkout [astronomer#1171](https://github.com/astronomer/airflow/pull/1171)
   
   Ah. Did not know that. That's a game changer. Then if submodules work and if we stick to SHA links in our .submodule, I am also fine with submodule.
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] ashb commented on issue #13513: Switch external actions to subrepo'ed .github/actions/

Posted by GitBox <gi...@apache.org>.

ashb commented on issue #13513:
URL: https://github.com/apache/airflow/issues/13513#issuecomment-755311799


   > > Submodules or subrepo almost the same reviewability - it still requires a PR to be merged and approved, and it is pinned to a specific commit.
   > 
   > Not really - you have to specifically go to the other repo in the right version and review it. That a lot more effort, you cannot leave comments etc. . With subrepo, the changed code is part of your PR and you review it together with other code. This makes perfect sense and it's much more natural.
   
   Github handles it nicer than that -- checkout https://github.com/astronomer/airflow/pull/1171


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk commented on issue #13513: Switch external actions to subrepo'ed .github/actions/

Posted by GitBox <gi...@apache.org>.

potiuk commented on issue #13513:
URL: https://github.com/apache/airflow/issues/13513#issuecomment-755303986


   > Submodules or subrepo almost the same reviewability - it still requires a PR to be merged and approved, and it is pinned to a specific commit.
   
   Not really - you have to specifically go to the other repo and review it. With subrepo, the changed code is part of your PR and you review it together with other code. This makes perfect sense and it's much more natural.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] ashb commented on issue #13513: Switch external actions to subrepo'ed .github/actions/

Posted by GitBox <gi...@apache.org>.

ashb commented on issue #13513:
URL: https://github.com/apache/airflow/issues/13513#issuecomment-755314052


   > Not really - you have to specifically go to the other repo in the right version and review it. That a lot more effort, you cannot leave comments etc. . With subrepo, the changed code is part of your PR and you review it together with other code. This makes perfect sense and it's much more natural.
   
   So, two things:
   
   > you cannot leave comments
   
   Yeah, you can -- https://github.com/astronomer/airflow/pull/1171 can have comments on it.
   
   > With subrepo, the changed code is part of your PR and you review it together with other code
   
   That's my _exact point_, and why I don't want the code in our repo: it is not code we want to write or maintain. Yes, we need to verify it, but we aren't writing that code ourselves, so we should not be reviewing that code for bugs/readability the same way we would a PR to airflow itself, so I don't even see where we need to leave comments on the actions code. Either the change is good and can be merged, or it's a change that needs making to the upstream action.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] ashb commented on issue #13513: Switch external actions to subrepo'ed .github/actions/

Posted by GitBox <gi...@apache.org>.

ashb commented on issue #13513:
URL: https://github.com/apache/airflow/issues/13513#issuecomment-755275805


   > But regardless of the capabilities, I believe this would be strictly against the problem INFRA tries to avold. The idea is to make sure action code is reviewed before it is used and we are not supposed to use code unreviewed by someone at apache.
   
   Submodules or subrepo almost the same reviewability - it still requires a PR to be merged and approved, and it is pinned to a specific commit.
   
   Let's see if it at least works: https://github.com/apache/airflow/pull/13514 - if it doesn't there is no point discussing this approach further.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] ashb commented on issue #13513: Switch external actions to subrepo'ed .github/actions/

Posted by GitBox <gi...@apache.org>.

ashb commented on issue #13513:
URL: https://github.com/apache/airflow/issues/13513#issuecomment-755263046


   No real opinion here.
   
   Could we use git submodules here? I normal avoid them, but if it works with Actions it might be an easy way to keep them separate but in out repo?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk commented on issue #13513: Switch external actions to subrepo'ed/submodules .github/actions/

Posted by GitBox <gi...@apache.org>.

potiuk commented on issue #13513:
URL: https://github.com/apache/airflow/issues/13513#issuecomment-758809892


   Closed by #13514  #13631 #13633


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk edited a comment on issue #13513: Switch external actions to subrepo'ed .github/actions/

Posted by GitBox <gi...@apache.org>.

potiuk edited a comment on issue #13513:
URL: https://github.com/apache/airflow/issues/13513#issuecomment-755268345


   > Reading https://github.community/t/github-action-defined-as-submodule-in-a-repo/139204/12 it looks like we can use git submodules for actions so long as ASF have allowed us to use official actions, which I think they have.
   > 
   > Thoughts on submodules here @potiuk ?
   
   I am not sure if it would work - especially parsing action inputs and verifying them at scheduler.  But regardless of the capabilities, I believe this would be strictly against the problem INFRA tries to avold. The idea is to make sure action code is reviewed before it is used and we are not supposed to use code unreviewed by someone at apache.
   
   The subrepo approach follows exactly this recommentdation that the code (and any change to the actions) MUST be reviewed before it gets used in the master ( and it is done without impacting our review workflow)
   
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk edited a comment on issue #13513: Switch external actions to subrepo'ed .github/actions/

Posted by GitBox <gi...@apache.org>.

potiuk edited a comment on issue #13513:
URL: https://github.com/apache/airflow/issues/13513#issuecomment-755268345


   > Reading https://github.community/t/github-action-defined-as-submodule-in-a-repo/139204/12 it looks like we can use git submodules for actions so long as ASF have allowed us to use official actions, which I think they have.
   > 
   > Thoughts on submodules here @potiuk ?
   
   I am not sure if it would work - especially parsing action inputs and verifying them at scheduler.  But regardless of the capabilities, I believe this would be strictly against the problem INFRA tries to avold. The idea is to make sure action code is reviewed before it is used and we are not supposed to use code unreviewed by someone at apache.
   
   The subrepo approach follows exactly this recommentdation that the code (and any change to the actions) MUST be reviewed before it gets used in the master 
   
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk commented on issue #13513: Switch external actions to subrepo'ed/submodules .github/actions/

Posted by GitBox <gi...@apache.org>.

potiuk commented on issue #13513:
URL: https://github.com/apache/airflow/issues/13513#issuecomment-755345620


   It works, so go ahead @ashb. It's much better with submodules!


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk commented on issue #13513: Switch external actions to subrepo'ed .github/actions/

Posted by GitBox <gi...@apache.org>.

potiuk commented on issue #13513:
URL: https://github.com/apache/airflow/issues/13513#issuecomment-755261879

CC: @kaxil @ashb @turbaszek @TobKed -> WDYT ? I think this is much better approach than the cloned repositories we have now.

Maybe @TobKed you would like to take a stab on it ? Happy to help as I've done it several times and know subrepo ins-outs very well.

a) when you want to update to newer version of the repo
b) when you want to upstream local change

And we worked out and documented that workflows nicely in one of the projects we worked on so we can bring the "step-by-step" instructions easily.

The nicest thing about this approach is that:

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] ashb edited a comment on issue #13513: Switch external actions to subrepo'ed .github/actions/

Posted by GitBox <gi...@apache.org>.

ashb edited a comment on issue #13513:
URL: https://github.com/apache/airflow/issues/13513#issuecomment-755264195


   Reading https://github.community/t/github-action-defined-as-submodule-in-a-repo/139204/12 it looks like we can use git submodules for actions so long as ASF have allowed us to use official actions, which I think they have.
   
   Thoughts on submodules here @potiuk?
   
   The main reason I'd prefer submodules over subrepo here is number of commits: having all the commit history for all the actions in our repo is "noisy", and coupuled with this only being used on CI everyone can basically ignore the fact we are using submodules -- development will happily work fine without them existing.
   
   So I guess I do have an opinion: no to subrepo or anything else that brings the commits for the actions in to our repo and "pollutes" our git history.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org