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

Long commit message on zipdeploy not being trimmed enough #288

Closed
IGx89 opened this issue Jan 25, 2023 · 10 comments
Closed

Long commit message on zipdeploy not being trimmed enough #288

IGx89 opened this issue Jan 25, 2023 · 10 comments

Comments

@IGx89
Copy link

IGx89 commented Jan 25, 2023

#271 added a length limit to the commit message, but it's still too short. I experimentally discovered that Kudu gives a 404 response for any URLs that have a query string longer than 2048 characters. The 7000 limit added in #271 is thus still much too long, it needs to be short enough that it, along with the rest of the query parameters and encoded JSON object, add up to less than 2048 characters. Guessing it should be less than 1000, since the trimming is done before the URL encoding which could more than double the effective length (" " becomes "%20", etc.).

@kumaraksh1
Copy link
Collaborator

@IGx89 are you facing any issue while deploying the changes ??
Like we tested it multiple times and it worked. But happy to work on it again if you are still facing any issues.

@IGx89
Copy link
Author

IGx89 commented Jan 30, 2023

Yep, we discovered this issue when doing a deployment last week. Zip deploy URL had 2572 characters, which was too long for Kudu. I experimented and dug through the code to determine the numbers in my description above.

What scenarios did you test? Do my findings above (Kudu's query string limit is 2048 characters and this action only trims commit messages longer than 7000 characters) seem accurate to you?

@github-actions
Copy link

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

@shpraka
Copy link
Contributor

shpraka commented Feb 24, 2023

@IGx89 I tried this and it's working with current constraint on the 7000. Do check the logs here: https://github.com/shpraka/NodeJsAppVer18/actions/runs/4262080360/jobs/7417199825.

Also can you please share the action run logs where we can see the full URI and check the length, it will help us debug further?

@Methuselah96
Copy link
Contributor

@shpraka Here is an example of it failing with the same commit message you used in your example: https://github.com/Methuselah96/ATestWebappsDeploy/actions/runs/4344485744/jobs/7587891966.

@github-actions
Copy link

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

@github-actions github-actions bot added the idle label Mar 20, 2023
@IGx89
Copy link
Author

IGx89 commented Mar 23, 2023

@shpraka would you still like me to provide an example to reproduce? I can't easily explain why it worked for you. What type of server was it running on? Windows? Linux? Mine was Windows/.NET, maybe yours is different enough that the web server hosting Kudu doesn't have the same 2048 character max query string limitation.

@shpraka
Copy link
Contributor

shpraka commented Mar 27, 2023

@IGx89 The app I was using for above example was on Linux.

@IGx89
Copy link
Author

IGx89 commented Mar 28, 2023

Thanks! Wouldn't be surprised at all if a web server on Linux allowed longer query string params than a Windows web server. I see a PR got merged that should fix this issue, thank you! I'll close this now, in anticipation of a new release coming soon, and will re-open if I encounter the issue again.

@IGx89 IGx89 closed this as completed Mar 28, 2023
@shpraka
Copy link
Contributor

shpraka commented Mar 29, 2023

@IGx89 We will be releasing the change soon, in a couple of weeks.

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

No branches or pull requests

4 participants