derberg / manage-files-in-multiple-repositories

GitHub Action that introduces support for global workflows. Global workflows are the one you update in just one repo and they are automatically updated in other repositories.
MIT License
51 stars 18 forks source link

feat: add option for topics_to_include and exclude_private #20

Closed RostiMelk closed 3 years ago

RostiMelk commented 3 years ago

Description

Results 55515880d7d68c5ca5889cbeb93d956c Here is an example with the new options enabled. I currently have a topic enabled for the repo Hyttenett. The rest of the repos are added to the ignoredRepositories array. Some repositories are blurred for confidentiality.

Related issue(s)

8

RostiMelk commented 3 years ago

With new filters being added, I think it is a good idea to move all the filters to its own class. That way the run function doesn't get as cluttered. What do you think @derberg ?

RostiMelk commented 3 years ago

Submitted new commit for cleaning up the new filters. I ended up placing them in a new file lib/filters.js.

RostiMelk commented 3 years ago

Thanks for the great feedback!

This is an amazing additional and high-quality PR man! I left only a few comments. Can you also please extend one of examples from readme with these new options https://github.com/derberg/global-workflows-support#advanced-workflow 🙏🏼

side question, private repos, is there a property for it too? when the list of repos is fetched? can I easily determine if repo is public or private?

Yes. You are able to check whether a repo is private or not in the API call. I did a quick test here:

{
id: 371025676,
node_id: 'MDEwOlJlcG9zaXRvcnkzNzEwMjU2NzY=',
name: 'wpe-template-helper',
full_name: 'designcontainer/wpe-template-helper',
private: false,
...
}

With new filters being added, I think it is a good idea to move all the filters to its own class. That way the run function doesn't get as cluttered. What do you think

Tbh, privately I'm not a fan of classes 😄 I use them only if they are really needed, otherwise composition with functions everywhere 😄 I agree though that the core of this action is not easy to digest. I chose to focus on good comments and super detailed logs. I never expected I will have to maintain this action for so long. I hoped to archive it over the native functionality provided by GitHub.....but looks like the adoption grows and even external code contributions are getting in 🎉 And if there will be a need to switch to classes because of it, let it be 😄

Yeah, A class might not be it. A new file was sufficient imo. 👍

RostiMelk commented 3 years ago

I ended up moving the filters to utils.js, because of an issue with exporting functions.

The followikg fixes that you mentioned has been done:

I have however not added any default state to topics. As I mentioned in a comment earlier; This will affect older existing setups, and exclude all their repositories.

RostiMelk commented 3 years ago

Since the GitHub Workflow inputs are treated as strings and not booleans, I had some issues where it would default to true even if false was set.

This was caused by using !!.

Here is a quick explanation of why it shouldn't be used for action inputs:

Any string which isn't the empty string will evaluate to true by using them. Although they're the cleanest methods I can think of concerning to boolean conversion, I think they're not what you're looking for.

Source: https://stackoverflow.com/questions/263965/how-can-i-convert-a-string-to-boolean-in-javascript#answer-264037

RostiMelk commented 3 years ago

The following changes has been done:

derberg commented 3 years ago

looks perfect, just update PR title and description and we are ready to merge and I'll release immediately