-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
[match] Added s3_skip_encryption parameter #21018
Conversation
Furthermore fixed a bug in parameters added to for_storage_mode
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Is there any way bump this 🥹 |
match/lib/match/nuke.rb
Outdated
@@ -55,6 +55,7 @@ def run(params, type: nil) | |||
s3_secret_access_key: params[:s3_secret_access_key].to_s, | |||
s3_bucket: params[:s3_bucket].to_s, | |||
s3_object_prefix: params[:s3_object_prefix].to_s, | |||
s3_skip_encryption: params[:s3_skip_encryption], | |||
gitlab_project: params[:gitlab_project], | |||
team_id: params[:team_id] || Spaceship::ConnectAPI.client.portal_team_id | |||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kinda dislike how the current design of for_storage_mode
forces all these unrelated params to be littered across all the call sites... what do you think about refactoring it so we just pass in the whole params
object plus any overrides and nil replacements? Then this entire block of code could be replaced with something like
self.storage = Storage
.from_params(params)
.replace_if_nil({team_id: Spaceship::ConnectAPI.client.portal_team_id})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine with me, I can give it a go. It was kinda more changes than I thought would be needed since I had to hand pick all params.
Stay tuned :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, so the params
object in this case is a FastlaneCore::Configuration
and that does not play nicely with the existing tests... I will see if it something I can work around.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@getaaron I gave it a swing, found that it made the most sense to cherry pick the params going into the Storage classes themselves since they are more specific.
Please give me your thoughts.
Added a few questions, also
what's the bug? I didn't notice it 😅 |
Co-authored-by: Aaron Brager <getaaron@gmail.com>
So the
|
@getaaron any chance of a second look at the changes? |
Anything I can do to get this PR rolling? |
Would be great to get this one in |
Possible to get some eyes on this? |
@mbogh I merged @darbyfrey Could you double-check the GitLab-related changes? For this part, I don't think there should be any behavior change, it's just refactoring to avoid sending unnecessary params. |
Nice improvements @mbogh! The GitLab storage mode changes look good. I've tested them locally and everything works as expected. |
@mbogh I think this is good to go except a few tests are still failing |
I will get it to green ❤️ |
@getaaron all green again |
@mbogh thanks very much for adding this feature to fastlane! |
Thanks for your work on Fastlane @getaaron 🤗 |
Hi, @mbogh @getaaron
I believe this has something to do with this change, but I have no Ruby experience to find the problem EDIT: |
@MichalAlgor thanks for the report — @darbyfrey said he can take a look today and we'll do a release shortly after |
@MichalAlgor apologies for the regression! I've got a fix ready in #21520. |
@darbyfrey no worries, glad I could help, thanks for quick fix 🙂 |
@@ -39,7 +89,8 @@ def register_backend(type: nil, storage_class: nil, &configurator) | |||
end | |||
end | |||
|
|||
def for_mode(storage_mode, params) | |||
def from_params(params) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like this change wasn't made backwards compatible 😬😬
This was brought to my attention in fastlane/docs#1225
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there anything I can do to help?
There was a problem hiding this 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 really a public API. The case in the docs (in the other PR) is a pretty niche use case / workaround for missing match functionality so I think this PR is probably fine. If anything we could add a test for the use case in the docs so we'd notice if we need to update them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah fair enough. Not sure if we have any guidelines on what to consider important to keep backwards compatible and what not to, so that's to blame haha
Whenever I personally make changes or review code I try to keep all APIs backwards compatible, but that may be exclusive to me and not really the team's preferences
I'll kick off a discussion around this in Slack so perhaps we can better define this :)
Thanks for the input guys!
Furthermore fixed a bug in parameters added to for_storage_mode
Checklist
bundle exec rspec
from the root directory to see all new and existing tests passbundle exec rubocop -a
to ensure the code style is validci/circleci
builds in the "All checks have passed" section of my PR (connect CircleCI to GitHub if not)Motivation and Context
We plan to store all certificates and provisioning profiles in S3 with custom tailored IAM Roles for each team/project/app that can be assumed via OIDC and it would be complicated and not so secure, to have to share a encryption key as well across GitHub, GitLab and other.
Description
Added a single parameter to disable/skip encryption if you are using S3 storage mode. I have had to add everywhere
Encryption.for_storage_mode(...
is invoked.In order to test this, I created two S3 buckets on my AWS account and ran:
bundle exec fastlane match import --skip_certificate_matching true -y appstore -b XYZ
withs3_skip_encryption(true)
and withs3_skip_encryption(false)
.Went into the AWS Console and browsed the S3 buckets, downloaded the imported files and could verify that the encrypted ones were in fact still encrypted and the unencrypted ones plain .cer, .p12.
Then I tested it from a lane:
Testing Steps
Matchfile
withbundle exec fastlane match import --skip_certificate_matching true -y appstore -b XYZ
s3_skip_encryption
and change bucketbundle exec fastlane match import --skip_certificate_matching true -y appstore -b XYZ