Skip to content

Comments

fix(server): improved packfile encoding to support short messages and…#409

Merged
JamieSlome merged 5 commits intofinos:mainfrom
sammoorhouse:main
Feb 6, 2024
Merged

fix(server): improved packfile encoding to support short messages and…#409
JamieSlome merged 5 commits intofinos:mainfrom
sammoorhouse:main

Conversation

@sammoorhouse
Copy link
Contributor

@sammoorhouse sammoorhouse commented Jan 22, 2024 •
edited
Loading

… unicode in errors

This change allows you to say

step.setError(`🥺 Rejecting repo ${action.repo} not in the authorisedList`);

in a step.

Also fixes a padding bug that meant fewer than 5chars of errors broke the proxy response.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jan 22, 2024 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@netlify
Copy link

netlify bot commented Jan 22, 2024 •
edited
Loading

✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.

Name Link
🔨 Latest commit d2d2c6b
🔍 Latest deploy log https://app.netlify.com/sites/endearing-brigadeiros-63f9d0/deploys/65c2513f9ecbe20008e7102c

@maoo
Copy link
Member

maoo commented Jan 23, 2024

Thanks for the PR @sammoorhouse ! Please reach out to [email protected] if you encounter any issue to sign the FINOS CLA using EasyCLA.

@JamieSlome
Copy link
Member

JamieSlome commented Jan 23, 2024

@sammoorhouse - thanks for the contribution 👍 I'd been wondering how to get some "prettier" messages into the CLI for the developer.

Do you think we could include a test case with this PR to demonstrate it works successfully? Perhaps to demonstrate that a message successfully returns with Unicode characters?

Happy to help if needed 🎉

@JamieSlome JamieSlome self-requested a review January 23, 2024 11:45
@JamieSlome JamieSlome added the bug Something isn't working label Jan 23, 2024
@sammoorhouse
Copy link
Contributor Author

sammoorhouse commented Jan 23, 2024

Thanks @JamieSlome I'll have a look later today!

@sammoorhouse
Copy link
Contributor Author

sammoorhouse commented Jan 23, 2024

I extracted 'handleMessage' in proxy/routes and wrote two tests just for that function in a second commit just now. Thanks!

JamieSlome
JamieSlome reviewed Jan 24, 2024
View reviewed changes
@JamieSlome JamieSlome mentioned this pull request Jan 24, 2024
Closed
psmulovics
psmulovics approved these changes Jan 25, 2024
View reviewed changes
@psmulovics
Copy link

psmulovics commented Jan 25, 2024

Can someone kick the build again? It failed on something unrelated imho.

@coopernetes
Copy link
Contributor

coopernetes commented Jan 25, 2024

Can someone kick the build again? It failed on something unrelated imho.

The failure is due to #213, we have some sort of repo permission bug in the coverage steps.

I’ll try to get a PR open soon to get it fixed then we can rebase on this one. Seems like a simple enough fix: zgosalvez/github-actions-report-lcov#150

@JamieSlome JamieSlome mentioned this pull request Jan 25, 2024
Merged
JamieSlome
JamieSlome approved these changes Jan 25, 2024
View reviewed changes
Copy link
Member

@JamieSlome JamieSlome left a comment

Choose a reason for hiding this comment

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

LGTM! 🍰

Ready to go once we've addressed the code coverage permissions issue...

(cc) @coopernetes @CyberCitizen01

@JamieSlome JamieSlome merged commit edeed3d into finos:main Feb 6, 2024
Psingle20 pushed a commit to Psingle20/git-proxy that referenced this pull request Nov 27, 2024
@JamieSlome
Merge pull request finos#409 from sammoorhouse/main
d364236 
fix(server): improved packfile encoding to support short messages and…
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JamieSlome JamieSlome JamieSlome approved these changes

+1 more reviewer

@psmulovics psmulovics psmulovics approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@sammoorhouse sammoorhouse

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@sammoorhouse @maoo @JamieSlome @psmulovics @coopernetes