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

Trim commit message to not include description #226

Merged
merged 1 commit into from
Sep 6, 2022

Conversation

Methuselah96
Copy link
Contributor

Resolves #192.

I considered trimming the commit message to a certain length, but it appears that it would be hard to completely predict how long the message sent to Kudu is since the message contains more than just the commit message. It seems reasonable to just include the first line of the commit message.

@ghost
Copy link

ghost commented Apr 5, 2022

CLA assistant check
All CLA requirements met.

@github-actions
Copy link

This PR is idle because it has been open for 14 days with no activity.

@mattnieland
Copy link

ANYONE gonna do anything about this problem and approve his PR?

@github-actions github-actions bot removed the idle label Jun 8, 2022
@github-actions
Copy link

This PR is idle because it has been open for 14 days with no activity.

@github-actions github-actions bot added the idle label Jun 22, 2022
@DavidBoike
Copy link

Can someone please look at this PR? This problem is a constant annoyance.

@github-actions
Copy link

This PR is idle because it has been open for 14 days with no activity.

@github-actions github-actions bot added the idle label Jul 29, 2022
@DavidBoike
Copy link

@Methuselah96 is there any way I can help? I can't even see why the CI failed because it was run so long ago, and I can't trigger it to re-run. Or are you only waiting for the Azure team to look at the PR?

@DavidBoike
Copy link

I tried opening #243 with the exact same changes (npm run build was run and npm run test passes all tests) in order to see what might be in the problem with the CI, but that results in "Waiting for review: automation test needs approval to start deploying changes." so there doesn't seem to be any point in having that PR.

It seems all this PR really needs is attention.

@Methuselah96
Copy link
Contributor Author

@DavidBoike Yeah, I'm just waiting on review, not sure what's up with the CI.

@github-actions github-actions bot removed the idle label Aug 1, 2022
Copy link

@darena-patrick darena-patrick left a comment

Choose a reason for hiding this comment

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

lgtm 🎉

@shubham805
Copy link
Contributor

@Methuselah96 How did you reproduce the 404 error (#192) with a long commit message?

@Methuselah96
Copy link
Contributor Author

@shubham805 The situation where we run into this is as described here. If you want to reproduce it without squash merge, you can just include a really long multi-line commit message (that includes a one line summary and then a really long description).

@shubham805
Copy link
Contributor

@Methuselah96 The PR looks good. Can you add a dummy commit just to re trigger the pr check pipelines?

@Methuselah96
Copy link
Contributor Author

@shubham805 Done, says the workflow is awaiting approval.

@shubham805
Copy link
Contributor

@Methuselah96 There were some issues in the PR check pipeline. We have updated it. Can you again add a new dummy commit, it will pick up the updated workflow file.

@Methuselah96 Methuselah96 temporarily deployed to automation test September 5, 2022 15:47 Inactive
@Methuselah96 Methuselah96 temporarily deployed to automation test September 5, 2022 15:47 Inactive
@Methuselah96
Copy link
Contributor Author

@shubham805 Done, awaiting approval again.

@shubham805 shubham805 merged commit 131b696 into Azure:master Sep 6, 2022
@Methuselah96 Methuselah96 deleted the trim-commit-message branch September 6, 2022 14:06
@Methuselah96
Copy link
Contributor Author

Methuselah96 commented Oct 7, 2022

@shubham805 Are there plans to release this fix? Looking forward to this being resolved.

@shpraka
Copy link
Contributor

shpraka commented Nov 28, 2022

@Methuselah96 this is released. Do let us know in case you find any issues.

@IGx89
Copy link

IGx89 commented Jan 25, 2023

We just ran into this on v2 -- appears to maybe not be fixed for every scenario? Using a self-hosted runner. The ZIP deploy URL was 2572 characters long...

@IGx89
Copy link

IGx89 commented Jan 25, 2023

Maybe the change should have been .split(/(\\r)?\\n/)[0] instead of .split(/\r?\n/)[0]? The latter doesn't work when I test it against a commit message such as "abc\nxyz\r\n123\n" (the commit message string contains the escape characters, so a newline for example is two characters: backslash and n)

@Methuselah96
Copy link
Contributor Author

This was replaced by #271 anyway, so the code in this PR is no longer used (although I agree it's wrong).

@IGx89
Copy link

IGx89 commented Jan 25, 2023

Should I submit a new issue, about 2572 characters in the URL being too long? #271 assumes that up to 8000 (+ change) characters is acceptable, which from our current experience seems inaccurate.

@Methuselah96
Copy link
Contributor Author

Yeah, that would be great, thanks!

@IGx89
Copy link

IGx89 commented Jan 25, 2023

Done: #288

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Getting 404 when deploying
7 participants