Diff of YML implementations #10

MichaelSp · 2021-07-30T09:02:33Z

Thanks for your early response (very appreciated!), your explanation #9 and for pointing me to the yml branch.

I found your action on the marketplace while looking for exactly that. We try to make everything-as-code/data(tm), so teams on GH must be covered, too!

I will try to compare the implementations so we can merge them later, like you suggested.

Overview of what this PR changes compared to support-yaml-team-configuration:

⚡rename repo-token to just token, since it's not really related to the repo.
✂️ split main.ts into 2 new files.
🐞 adding test for new code
🆙 update dependencies
🧹 eslint fixed

I've moved some of your code into this PR, so we can also close #9

…n-files' # Conflicts: # .eslintrc.json # README.md # action.yml # dist/index.js # package-lock.json # package.json # src/main.ts

rmacklin · 2021-07-30T20:18:01Z

Ah, I think I wasn't totally clear before, so apologies for that! I'm going to merge support-yaml-team-configuration-files directly, given that that's already what the v0 tag has been pointing to since February, which means everyone who followed the README examples has already been depending on that code for quite a while. So, it's not necessary to target a PR into the support-yaml-team-configuration-files branch, and I think we'll be better off avoiding an attempt to non-linearly merge these two commit histories.

What I propose we do to move forward is split out separate pieces of your changes into a few smaller PRs that are branched out from the current v0 code, since it'll be easier to review and merge the separate pieces incrementally. I think the best change to start with would be just adding the test coverage before we make any changes to the action code. That'll give us confidence that we're protected against regressions when making subsequent changes to the action itself. Then we can follow with individual PRs that change the action itself (renaming the token input, upgrading dependencies, etc.). This will also make it easier to revert an individual change if, for example, one of the dependency upgrades breaks something (that's probably/hopefully unlikely, but I think it's a good example to illustrate one of the benefits of splitting out each separate change).

To be clear, I'm happy to help execute on all of this - I'll free up some time to revisit this project, thanks to your reminder - so if any of that sounds overwhelming, don't hesitate to reach out about what you're comfortable contributing!

rmacklin · 2021-07-30T21:59:15Z

Ok, I merged the support-yaml-team-configuration-files branch into a new main branch because I also wanted to switch over to GitHub's newer default branch naming convention, so that's the new default branch (therefore new PRs should be based on main). I also went ahead and merged in all the dependabot PRs that were open.

Given that this PR now has conflicts in package-lock.json:

I'm going to close it and then delete the obsolete master branch. But you can still reply to my last comment here to continue the discussion about the next steps.

MichaelSp · 2021-08-04T16:47:06Z

You can reopen this. I've pushed a commit that should resolve the conflicts.

MichaelSp · 2021-08-05T19:31:19Z

package.json

-    "@actions/github": "^2.1.1",
-    "@octokit/rest": "^16.43.1",
+    "@actions/core": "^1.4.0",
+    "@actions/github": "^5.0.0",


The update from 2.1.1 to 5.0.0 is really critical for me since it doesn't work on Github enterprise without that:

https://github.com/actions/toolkit/blob/main/packages/github/RELEASES.md#220

Got it - thanks for explaining that! Based on the changelog link you provided, it seems like upgrading to 2.2.0 is sufficient, so I want to start there without switching to a new major version yet (since the major version upgrades could include breaking changes). I've opened a new PR for that upgrade: #14.

Can you give it a try with

steps: - uses: rmacklin/team-sync@5617c3b5dd662e0b0b35f93bbf4cb46b416c7b0b

in your workflow definition, and let me know if it works on your GitHub Enterprise Server? If so, I'll merge and push a new release.

I'm already running it on my master branch (which is subject of this PR). So no need for an intermediate step.

I face however a very strange timing issue. I try to sync ~20 teams. but it always fails after the ~15th team with the following error:

Error: HttpError: request to https://<GHES-API>/teams failed, reason: connect ETIMEDOUT
Not sure how to proceed. Especially since it's working ~14 times and then fails...

Okay, I've merged #14.

I haven't seen that error personally, but I don't use GitHub Enterprise. Using the action with an organization on github.com, I've seen it sync over 40 teams successfully. It seems possible that a self-hosted GitHub Enterprise deployment may not be able to handle the volume of successive requests to query for team memberships and then add/remove team members accordingly. That's just a guess, though, so I'd suggest cross referencing any monitoring you have in place for the GitHub Enterprise deployment to look for indications that it's timing out because it's unable to handle the load.

To rule out code/dependency changes, you can try reproducing using rmacklin/team-sync@5617c3b5dd662e0b0b35f93bbf4cb46b416c7b0b rather than the fork, but it seems more likely that it's an issue with the GitHub Enterprise deployment.

I just tried rmacklin/team-sync@main: Doesn' t work:

Ok, I think it might be necessary to upgrade to @actions/github to v3.0.0 to pull in actions/toolkit@4a89cf7 (upgrading further includes breaking changes to Octokit, which I'd like to handle separately).

I've upgraded to v3.0.0 here: 042d3b8. If you update your mirror, you can try again with
uses: ghcom-actions/rmacklin-team-sync@042d3b844823ea5ae2a03c23ea55e1c822deab51.

(I do still suspect that it's more likely an issue with the GitHub Enterprise deployment being overloaded, but this may at least get past the "Bad credentials" error.)

Must be something on the runner. When running locally it worked on my MichaelSp/team-sync@master branch.
Haven't checked your branch.

Sorry, but I have to say: I don't see a reason to break this down into pieces.
Why:

If you write the tests first, they would be for the old API, so you gonna write them twice.

If you do the upgrade first, you could break things, since you have no tests...

Conclusion: Happy with what I have now.

Digging into the runner now, to see what is wrong there....

MichaelSp added 4 commits July 29, 2021 11:27

read yml files; adding tests

ffa1f03

add missing type team_sync_ignored

d1004bf

handle empty fields properly

871b2da

fix es-lint 🧹

758b638

MichaelSp mentioned this pull request Jul 30, 2021

read yml files #9

Closed

MichaelSp added 2 commits July 30, 2021 11:07

Merge remote-tracking branch 'upstream/support-yaml-team-configuratio…

9ac4d5a

…n-files' # Conflicts: # .eslintrc.json # README.md # action.yml # dist/index.js # package-lock.json # package.json # src/main.ts

fix merge conflicts

a2f2246

rmacklin changed the base branch from support-yaml-team-configuration-files to master July 30, 2021 20:18

rmacklin changed the base branch from master to support-yaml-team-configuration-files July 30, 2021 20:18

rmacklin mentioned this pull request Jul 30, 2021

Support YAML team configuration files #11

Merged

rmacklin deleted the branch rmacklin:obsolete July 30, 2021 20:23

rmacklin closed this Jul 30, 2021

rmacklin reopened this Jul 30, 2021

rmacklin changed the base branch from support-yaml-team-configuration-files to master July 30, 2021 20:24

rmacklin marked this pull request as draft July 30, 2021 20:24

rmacklin closed this Jul 30, 2021

MichaelSp commented Aug 5, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Diff of YML implementations #10

Diff of YML implementations #10

MichaelSp commented Jul 30, 2021 •

edited

Loading

rmacklin commented Jul 30, 2021 •

edited

Loading

rmacklin commented Jul 30, 2021 •

edited

Loading

MichaelSp commented Aug 4, 2021

MichaelSp Aug 5, 2021

rmacklin Aug 6, 2021 •

edited

Loading

MichaelSp Aug 8, 2021 •

edited

Loading

rmacklin Aug 11, 2021 •

edited

Loading

MichaelSp Aug 11, 2021

rmacklin Aug 12, 2021

MichaelSp Aug 16, 2021

Diff of YML implementations #10

Diff of YML implementations #10

Conversation

MichaelSp commented Jul 30, 2021 • edited Loading

rmacklin commented Jul 30, 2021 • edited Loading

rmacklin commented Jul 30, 2021 • edited Loading

MichaelSp commented Aug 4, 2021

MichaelSp Aug 5, 2021

Choose a reason for hiding this comment

rmacklin Aug 6, 2021 • edited Loading

Choose a reason for hiding this comment

MichaelSp Aug 8, 2021 • edited Loading

Choose a reason for hiding this comment

rmacklin Aug 11, 2021 • edited Loading

Choose a reason for hiding this comment

MichaelSp Aug 11, 2021

Choose a reason for hiding this comment

rmacklin Aug 12, 2021

Choose a reason for hiding this comment

MichaelSp Aug 16, 2021

Choose a reason for hiding this comment

MichaelSp commented Jul 30, 2021 •

edited

Loading

rmacklin commented Jul 30, 2021 •

edited

Loading

rmacklin commented Jul 30, 2021 •

edited

Loading

rmacklin Aug 6, 2021 •

edited

Loading

MichaelSp Aug 8, 2021 •

edited

Loading

rmacklin Aug 11, 2021 •

edited

Loading