| 04:23 | <ljharb> | looks like about 3 weeks ago, github automatically turned on "environment protection" which only allows protected branches to deploy, and gh-pages isn't a protected branch. i loosened the setting and reran all the failed deploy jobs, and things should now be up to date, and moving forward. nice catch! lmk if you see any other issues. |
| 05:28 | <Michael Ficarra> | is there a reason gh-pages isn't protected? |
| 05:28 | <Michael Ficarra> | would it make sense to just protect it instead? |
| 05:44 | <ljharb> | we can do that too - we don't want deploys from just any protected branch, so either way the settings should only allow deploying from gh-pages |
| 05:45 | <ljharb> | but, protecting the branch would likely block the automation that pushes to it. |
| 15:28 | <Chris de Almeida> | if it's literally just "protected branch" with no specific requirements, you can just do something really lightweight, like just checking "Do not allow bypassing the above settings" and not checking anything else |
| 15:29 | <Chris de Almeida> | or like, 'allow force pushes' from one individual |
| 20:48 | <ljharb> | psa, the IPR thing happened again; i'm working on fixing the bug in the script now so it'll catch it before merge |
| 20:51 | <bakkot> | lol the contributor is not willing to sign the thing |
| 20:52 | <bakkot> | it's a totally trivial contribution; I would personally be inclined to say it doesn't need the IPR form |
| 20:52 | <bakkot> | but IIRC littledan was of the opinion that everyone needs to sign it |
| 20:53 | <bakkot> | I guess we back it out and land exactly the same commit but attributed to me? with maybe attribution given in the commit message? |
| 21:01 | <shu> | wait |
| 21:01 | <shu> | the contributor is not willing to sign it? |
| 21:02 | <bakkot> | correct |
| 21:02 | <bakkot> | and the commit is already merged |
| 21:11 | <ljharb> | there's 3 PRs total, in fact |
| 21:11 | <ljharb> | so yeah we'd need to back all those out, and then we could reland them under someone else's name since they're indeed trivial |
| 21:11 | <ljharb> | i put up https://github.com/tc39/ecma262/pull/3128 which i think will catch the issue of a commit using an email address not associated with a github account, which was the problem with these three |
| 21:12 | <ljharb> | Michael Ficarra: do we really need to wait until wednesday to land that PR? is it something we could talk out here? |
| 21:16 | <Michael Ficarra> | I don't like that we're using a variety of different allowlisting strategies |
| 21:16 | <Michael Ficarra> | and was already uncomfortable that they were imprecise before |
| 21:16 | <ljharb> | the goal is to reduce them all to zero |
| 21:17 | <Michael Ficarra> | we're not going to reduce them all to zero, there are legacy contributions |
| 21:17 | <ljharb> | in the case of the commit one tho, those can't ever be fixed unless the email address on the commit is added (or re-added) to the user's github account. |
| 21:17 | <ljharb> | the form-signing one we absolutely can. but indeed the legacy commit list likely couldn't. for example, bterlson has commits in there with his microsoft email address |
| 21:17 | <Michael Ficarra> | is there any reason to do anything other than hard-code some SHAs? |
| 21:17 | <ljharb> | it would be a large list of shas. but sure, we could do that |
| 21:18 | <ljharb> | but i'm not sure why that needs to hold up this PR, which fixes a current issue, since we can iterate on it |
| 21:18 | <Michael Ficarra> | because I'm tired of it breaking like this |
| 21:18 | <Michael Ficarra> | this is embarrassing |
| 21:18 | <ljharb> | this PR fixes that breakage |
| 21:18 | <Michael Ficarra> | I cannot be sure of that until I understand the solution better |
| 21:18 | <ljharb> | (modulo the 3 PRs discussed above, which it can account for as well) |
| 21:19 | <ljharb> | ok, how can i help you understand that? basically, a commit can have any email address on it, and github only links that to an account if the email address is currently on the account |
| 21:19 | <ljharb> | and the "legacy commit list", including the commits that broke it this time, are all commits that can't be linked to an account |
| 21:20 | <bakkot> | contributing.md should probably mention in big letters that the IPR form is required, also |
| 21:20 | <bakkot> | it doesn't currently afaict |
| 21:20 | <ljharb> | but either way what is the harm of adding this now? it won't make things worse, and more importantly it won't make it any harder to come up with an alternative solution later |
| 21:21 | <ljharb> | as it stands now, PRs that will break main don't show up as broken until after merging; with this PR, those PRs will now be blocked from landing |
| 21:21 | <ljharb> | i'm personally more concerned with putting out the current fire than with embarrassment. |
| 21:24 | <ljharb> | i'll add the revert commits for those three PRs to my IPR fix, though, since they clearly aren't going to sign it |
| 21:25 | <Michael Ficarra> | we could also just set a starting date? |
| 21:26 | <Michael Ficarra> | and it can just be bumped to the last "manual audit" in case of failure |
| 21:26 | <ljharb> | sure, that would be fine too. landing this PR doesn't make it harder to make that change tho. |
| 21:32 | <ljharb> | my fix PR now reverts those 3 PRs, as well as fixes the current issue (altho it won't show up as fixed til it lands, because it uses pull_request_target), and prevents it from reoccurring, so i'd prefer to land that now and we can change the strategy to whatever you want later |