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

feat(publisher-github): assets management #1761

Merged
merged 7 commits into from Sep 25, 2023
Merged

Conversation

mahnunchik
Copy link
Contributor

  • I have read the contribution documentation for this project.
  • I agree to follow the code of conduct that this project follows, as appropriate.
  • The changes are appropriately documented (if applicable).
  • The changes have sufficient test coverage (if applicable).
  • The testsuite passes successfully on my local machine (if applicable).

Summarize your changes:

This PR includes fixes related to publisher-gihub:

@mahnunchik
Copy link
Contributor Author

It would be awesome to remove deprecation warning by merging this PR: #1673 But there are broken tests.

@mahnunchik
Copy link
Contributor Author

@malept is it possible to have it merged?

@mahnunchik
Copy link
Contributor Author

@malept What should I do to have this PR merged?

malept
malept previously requested changes Nov 16, 2020
Copy link
Member

@malept malept left a comment

Choose a reason for hiding this comment

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

I'd prefer if there were some tests around name sanitization.

Also the PR title needs to be fixed to be conventional commits-formatted

@mahnunchik mahnunchik changed the title publisher-github: assets management feat (publisher-github): assets management Dec 18, 2020
@codecov
Copy link

codecov bot commented Dec 18, 2020

Codecov Report

Merging #1761 (e15c362) into master (4824af6) will increase coverage by 12.45%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1761       +/-   ##
===========================================
+ Coverage   60.28%   72.73%   +12.45%     
===========================================
  Files          71       74        +3     
  Lines        2153     2223       +70     
  Branches      411      420        +9     
===========================================
+ Hits         1298     1617      +319     
+ Misses        676      446      -230     
+ Partials      179      160       -19     
Impacted Files Coverage Δ
packages/publisher/github/src/util/github.ts 100.00% <100.00%> (ø)
packages/utils/async-ora/src/index.ts 100.00% <0.00%> (ø)
packages/maker/wix/src/MakerWix.ts 86.36% <0.00%> (ø)
packages/maker/zip/src/MakerZIP.ts 100.00% <0.00%> (ø)
packages/maker/squirrel/src/MakerSquirrel.ts 77.27% <0.00%> (ø)
packages/utils/test-utils/src/index.ts 85.71% <0.00%> (+3.10%) ⬆️
packages/utils/async-ora/src/ora-handler.ts 90.00% <0.00%> (+3.33%) ⬆️
packages/maker/deb/src/MakerDeb.ts 100.00% <0.00%> (+5.26%) ⬆️
packages/maker/rpm/src/MakerRpm.ts 100.00% <0.00%> (+5.26%) ⬆️
packages/maker/dmg/src/MakerDMG.ts 100.00% <0.00%> (+5.55%) ⬆️
... and 21 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4824af6...e15c362. Read the comment docs.

@mahnunchik
Copy link
Contributor Author

@malept any chance to have this PR merged?

@mahnunchik
Copy link
Contributor Author

@malept ping again

@erickzhao
Copy link
Member

Just rebased this with latest main

@erickzhao erickzhao requested review from malept and a team November 7, 2022 22:09
Copy link
Member

@erickzhao erickzhao left a comment

Choose a reason for hiding this comment

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

I don't think this is shippable with the current GitHub.sanitizeName implementation. I tried looking for updated documentation and found this paragraph in their docs on uploading release assets:

GitHub renames asset filenames that have special characters, non-alphanumeric characters, and leading or trailing periods. The "List assets for a release" endpoint lists the renamed filenames. For more information and help, contact GitHub Support.

I couldn't find any additional details on the replacement actual algorithm used, but it doesn't seem to match the sanitizeName regex replacements based on my tests uploading release assets through the GitHub UI.

Raw filename sanitizeName GitHub UI
erick@256éoooz!.txt erick-256e-oooz-.txt erick@2aa.56eoooz.txt
fake. file.txt fake.file.txt fake.file.txt
erick%%%%%!!!zhao.txt erick--------zhao.txt erick.zhao.txt

The weird thing is that @ is not alphanumeric but does not get sanitized by GitHub. The opaqueness of their algorithm makes it hard to write exact and accurate replacement code.

Proposal

Instead of trying to dealing with the black box sanitization algorithm, do we want to force delete all assets upon --force before republishing?

@mahnunchik
Copy link
Contributor Author

@erickzhao deleting all assets is not suitable for me because assets uploaded per platform, so multiple publishers will remove assets of each other.

@mahnunchik
Copy link
Contributor Author

@erickzhao I think it is ok to ship sanitizeName as is and fix it in case of incorrect name generation in the future.

@mahnunchik
Copy link
Contributor Author

@erickzhao could we have it merged? Should I rebase PR?

@erickzhao erickzhao changed the title feat (publisher-github): assets management feat(publisher-github): assets management Sep 19, 2023
Copy link
Member

@erickzhao erickzhao left a comment

Choose a reason for hiding this comment

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

I think this is still a net improvement over the previous implementation.

@mahnunchik
Copy link
Contributor Author

Could it be merged?

@mahnunchik
Copy link
Contributor Author

@erickzhao @malept I've rebased on top of current main branch. Could the PR be merged or should I change something?

@VerteDinde VerteDinde merged commit eb7a919 into electron:main Sep 25, 2023
7 checks passed
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.

None yet

6 participants