Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Owner being replaced in github actions #1073

Conversation

jaas666
Copy link
Contributor

@jaas666 jaas666 commented Dec 3, 2023

PR Checklist

Overview

This PR should fix #1043 Originally I wanted to go with a callback function to exclude certain strings from being replaced but that's above my pay grade.

I don't like the way we are excluding using regex but this should solve the issue in the meantime.

Someday I'll get to that level. 🤓

@jaas666 jaas666 changed the title Fix owner being replaced in github actions fix: Owner being replaced in github actions Dec 3, 2023
@jaas666 jaas666 marked this pull request as draft December 3, 2023 02:37
@jaas666 jaas666 marked this pull request as ready for review December 3, 2023 03:35
@jaas666 jaas666 marked this pull request as draft December 3, 2023 05:46
@jaas666 jaas666 marked this pull request as ready for review December 3, 2023 06:10
Copy link

codecov bot commented Dec 3, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c347e0b) 94.18% compared to head (1a6a0c9) 94.18%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1073   +/-   ##
=======================================
  Coverage   94.18%   94.18%           
=======================================
  Files          96       96           
  Lines        5571     5574    +3     
  Branches      448      448           
=======================================
+ Hits         5247     5250    +3     
  Misses        323      323           
  Partials        1        1           
Flag Coverage Δ
create 70.60% <0.00%> (-0.04%) ⬇️
initialize 41.87% <100.00%> (+0.03%) ⬆️
migrate 69.82% <100.00%> (+0.01%) ⬆️
unit 72.08% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jaas666 jaas666 force-pushed the jaas666/fix-owner-being-replaced-in-actions branch from c635d4e to 1a6a0c9 Compare December 3, 2023 06:17
Copy link
Owner

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I see how you ended up here but:

  • This isn't super scalable: there are likely going to be more and more actions over time.
  • There's an action mentioned in the issue and not covered here, release-it-action

I think the callback method -or something else that doesn't create long regex- would be the way to go.

@JoshuaKGoldberg JoshuaKGoldberg added the status: waiting for author Needs an action taken by the original poster label Dec 4, 2023
@jaas666
Copy link
Contributor Author

jaas666 commented Dec 4, 2023

I agree. I don't like this solution either.

I tried coming up with a solution using the callback which should work but I couldn't figure out the correct regex to use.

I know my problem was that I change the regex to match /JoshuaKGoldberg/g only so the function was always false.

This , along with a new excludedFromReplacements variable, was my original attempt. A clever regex should make this work right?

[
/JoshuaKGoldberg/g,(match: string) =>
excludedFromReplacements.some((e) =>
match.includes(e))
? Match
: options.owner,
]

@JoshuaKGoldberg
Copy link
Owner

Instead of /JoshuaKGoldberg/g, maybe /JoshuaKGoldberg(\/\w+)?/? That way it also captures what comes after the JoshuaKGoldberg/, if anything.

@jaas666
Copy link
Contributor Author

jaas666 commented Dec 6, 2023

Sorry. I get and F on this one. I've tried but got nowhere. I'm adding release-it-action to at least make the PR functional, but feel free to close it until someone can properly fix it if you want.

@JoshuaKGoldberg
Copy link
Owner

You got it - thanks for trying though! I really appreciate you putting the effort in. ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting for author Needs an action taken by the original poster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 Bug: Migrating a non-JoshuaKGoldberg repo creates the wrong owners for JoshuaKGoldberg actions
2 participants