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-gcs): add Google Cloud Storage publisher #2100

Merged
merged 9 commits into from
Nov 16, 2023

Conversation

mahnunchik
Copy link
Contributor

Continue of #1752

@codecov
Copy link

codecov bot commented Dec 18, 2020

Codecov Report

Merging #2100 (593f2c3) into master (7050550) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2100   +/-   ##
=======================================
  Coverage   72.71%   72.71%           
=======================================
  Files          74       74           
  Lines        2221     2221           
  Branches      420      420           
=======================================
  Hits         1615     1615           
  Misses        446      446           
  Partials      160      160           

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 7050550...593f2c3. Read the comment docs.

@mahnunchik
Copy link
Contributor Author

Hi @malept could you please review this PR.

@mahnunchik
Copy link
Contributor Author

@malept 😢

@mahnunchik
Copy link
Contributor Author

Hi @malept when my PR may be accepted?

Copy link

@patgpt patgpt left a comment

Choose a reason for hiding this comment

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

A review was required.

@erickzhao erickzhao self-requested a review August 11, 2022 19:29
@mahnunchik
Copy link
Contributor Author

@erickzhao What about Google Cloud Storage publisher? I really need it for my workflow

.github/CODEOWNERS Outdated Show resolved Hide resolved
packages/publisher/gcs/package.json Outdated Show resolved Hide resolved
packages/publisher/gcs/package.json Outdated Show resolved Hide resolved
packages/publisher/gcs/src/PublisherGCS.ts Outdated Show resolved Hide resolved
packages/publisher/gcs/src/PublisherGCS.ts Outdated Show resolved Hide resolved
@mahnunchik
Copy link
Contributor Author

@MarshallOfSound @erickzhao @BlackHole1 I've decided to rebase on top of current main and squash to single commit.

@mahnunchik
Copy link
Contributor Author

Should I rebase PR again?

@erickzhao erickzhao changed the title feat(publisher-gcs): implemented Google Cloud Storage publisher feat(publisher-gcs): add Google Cloud Storage publisher Dec 21, 2022
packages/publisher/gcs/src/PublisherGCS.ts Outdated Show resolved Hide resolved
packages/publisher/gcs/src/PublisherGCS.ts Outdated Show resolved Hide resolved
packages/publisher/gcs/src/PublisherGCS.ts Outdated Show resolved Hide resolved
packages/publisher/gcs/src/PublisherGCS.ts Show resolved Hide resolved
packages/publisher/gcs/src/PublisherGCS.ts Outdated Show resolved Hide resolved
packages/publisher/gcs/src/PublisherGCS.ts Outdated Show resolved Hide resolved
packages/publisher/gcs/src/PublisherGCS.ts Outdated Show resolved Hide resolved
packages/publisher/gcs/src/PublisherGCS.ts Outdated Show resolved Hide resolved
Comment on lines 9 to 30
keyFilename?: string;
/**
* The Google Cloud project ID.
*
* Defaults to the value in the `GOOGLE_CLOUD_PROJECT` environment variable.
* */
projectId?: string;
/**
* The email for your Google service account, *required* when using a PEM/PKCS #12-formatted
* file in the [[keyFilename]] option.
*
* Defaults to the value in the `GOOGLE_CLOUD_CLIENT_EMAIL` environment variable.
*/
clientEmail?: string;
/**
* The private key for your Google service account.
*
* Defaults to the value in the `GOOGLE_CLOUD_PRIVATE_KEY` environment variable.
*/
privateKey?: string;
Copy link
Member

Choose a reason for hiding this comment

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

comment: For these auth-related options, can we put them in a separate auth object?

Copy link
Member

@erickzhao erickzhao Oct 20, 2023

Choose a reason for hiding this comment

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

I think the previous comment in this chain is still a valid concern. It might be cleaner to put all auth-related private key config options under an auth property.

packages/publisher/gcs/src/Config.ts Show resolved Hide resolved
@mahnunchik
Copy link
Contributor Author

@erickzhao I was able to rebase PR on top of current main by fighting the dependency hell... I make GCS publisher looks like S3 publisher and ported changes from it.

Could you please help me to ship GCS publisher before main have many new changes?

@nikashitsa
Copy link

Useful feature for me also.

@mahnunchik
Copy link
Contributor Author

@erickzhao could I ask for a review of this PR?

@mahnunchik
Copy link
Contributor Author

@erickzhao I've rebase PR on top of current changes. Could you please tell me what should I do to have this PR merged?

@mahnunchik
Copy link
Contributor Author

I'm using this publisher for a years, published by my package name. Could we make this PR merged?

@mahnunchik
Copy link
Contributor Author

@MarshallOfSound @erickzhao I need this publisher in the application. What can I do to get the PR accepted?

@mahnunchik
Copy link
Contributor Author

@MarshallOfSound @erikian @erickzhao ping

@erikian erikian requested a review from a team October 18, 2023 00:03
@BlackHole1
Copy link
Member

BlackHole1 commented Oct 18, 2023

Hi @mahnunchik . I saw @erickzhao commented on some issues. If they have been resolved, you can click the “Resolve conversation” button. If you have any questions regarding the issues, you can comment below. This will help other maintainers with the code review.

@mahnunchik
Copy link
Contributor Author

@BlackHole1 @erickzhao All open conversations was commented by me. GCS publisher is based on S3 publisher, I think both should be close as possible.

@erickzhao erickzhao self-requested a review October 20, 2023 20:47
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.

Left two minor comments. Also, our policy is generally to only let maintainers commit lockfile changes. Can you also revert yarn.lock here @mahnunchik?

packages/publisher/gcs/package.json Outdated Show resolved Hide resolved
Comment on lines 9 to 30
keyFilename?: string;
/**
* The Google Cloud project ID.
*
* Defaults to the value in the `GOOGLE_CLOUD_PROJECT` environment variable.
* */
projectId?: string;
/**
* The email for your Google service account, *required* when using a PEM/PKCS #12-formatted
* file in the [[keyFilename]] option.
*
* Defaults to the value in the `GOOGLE_CLOUD_CLIENT_EMAIL` environment variable.
*/
clientEmail?: string;
/**
* The private key for your Google service account.
*
* Defaults to the value in the `GOOGLE_CLOUD_PRIVATE_KEY` environment variable.
*/
privateKey?: string;
Copy link
Member

@erickzhao erickzhao Oct 20, 2023

Choose a reason for hiding this comment

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

I think the previous comment in this chain is still a valid concern. It might be cleaner to put all auth-related private key config options under an auth property.

@erickzhao erickzhao self-requested a review November 1, 2023 21:53
@erickzhao erickzhao self-assigned this Nov 1, 2023
@erickzhao
Copy link
Member

Here are the tweaks I propose: all the auth-related Storage constructor options should be nested in their own property:

mahnunchik#364

This way we aren't limiting ourselves to a subset of the credential options available via the @google-cloud/storage module, and it's less confusing to have all these options available at the top-level publisher config.

@mahnunchik
Copy link
Contributor Author

@erickzhao Thank you for your help! Implementation has become much simpler and clearer.

Should I close conversations above? I think everything is fixed now.

This should be a transient dep via `@google-cloud/storage`
@erickzhao
Copy link
Member

I think everything is fixed! We're planning on releasing a new Forge version within the next week, so hopefully this can make its way in.

@erickzhao
Copy link
Member

Rebased after the 7.0.0 breaking change release. Once CI is green, going to merge this and get it into a 7.1.0 release before the end of the month.

@erickzhao erickzhao merged commit 601314e into electron:main Nov 16, 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.

7 participants