probot / octokit-plugin-config

🛠️ Get/set persisted configuration using YAML/JSON files in repositories
ISC License
35 stars 16 forks source link

fix: resolve _extends in .github repo file #167

Closed mkurz closed 2 years ago

mkurz commented 2 years ago

Fixes #166

If this pull request get merged I would highly appreciate a new release :wink:

jetersen commented 2 years ago

The logic is somewhat flawed 😓

I am sure it is possible to rewrite it to simply loop over each next config object until no more _extends exist. Sounds like a enumerator with moveNext 😅

order also matters, requestedRepoFile should always be the first entry.

Than any extends? and last .github with it's extends?

Could also be split out to resolve requestedRepoFile first and than resolve .github (first check if it is already loaded some might have used _extends: .github).

mkurz commented 2 years ago

The logic is somewhat flawed

To be honest I don't think the logic is flawed.

order also matters, requestedRepoFile should always be the first entry.

Order is correct. requestedRepoFile will always be first, the array push(...) method always appends at the end. So this is correct.

mkurz commented 2 years ago

Let's see what @gr2m thinks.

gr2m commented 2 years ago

Let's see what @gr2m thinks.

sorry @gr2m is very busy right now, this will require more time and attention that I currently have available. If the two of you agree, I'd be okay with merging it

Thanks a lot @jetersen for the reviews and comments

mkurz commented 2 years ago

@gr2m yes please merge. We were discussing code style, but the logic is correct. Could you please also release a new version? Thanks!

mkurz commented 2 years ago

@gr2m You need to also approve the workflows to run for that pull request.

mkurz commented 2 years ago

@gr2m Fixed the formatting, please approve again, thanks!

mkurz commented 2 years ago

Good morning @gr2m, can you please approve again? Thanks!

mkurz commented 2 years ago

@gr2m Again please, it should work now! I finally ran the checks locally. Thanks!

mkurz commented 2 years ago

OK, now this: "Jest: "global" coverage threshold for branches (100%) not met: 94.92%"...

mkurz commented 2 years ago

@gr2m Test coverage should be fixed now as well... Sorry

mkurz commented 2 years ago

@gr2m Hopefully just one last time approving... :heart:

mkurz commented 2 years ago

@gr2m Alright, I now pushed a test which will fail. Holding back the fix and will commit after the test ran.

mkurz commented 2 years ago

@gr2m Once more your approval is needed.

mkurz commented 2 years ago

@gr2m Because it's late here I will go to bed. However, I just entered this into my command line:

$ sleep 7200 ; git push

which means around 6:18 pm L.A. time the implementation will be pushed, so I hope you approved until then, so afterwards you could approve the implementation as well, merge and release...

I know, it's a though plan :stuck_out_tongue_winking_eye:

mkurz commented 2 years ago

The plan didn't work, so I removed that second commit again for now :wink:

gr2m commented 2 years ago

You can push the fix now

mkurz commented 2 years ago

Pushed

mkurz commented 2 years ago

@gr2m Thank you! Can you release that fix?

github-actions[bot] commented 2 years ago

:tada: This PR is included in version 1.1.5 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

mkurz commented 2 years ago

Thank you very much!