-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
Explicit beta group access to all builds at creation #21478
Explicit beta group access to all builds at creation #21478
Conversation
Hi @vincentisambart 👋 could you merge master into your branch (or rebase it)? There was a fix to our CI that is in master that should make your PR pass all checks 🙏 Thank you! |
d4ec0c6
to
0a5c52e
Compare
Rebase done 😁 |
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.
The code change looks good, and thanks for rebasing. We should just add test coverage for this condition and then we'll be good to merge 👍
0a5c52e
to
c3a2f4a
Compare
has_access_to_all_builds = true if has_access_to_all_builds.nil? | ||
else | ||
# Access to all builds is only for internal groups | ||
has_access_to_all_builds = nil |
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 checked but creating a few beta groups on App Store Connect, and if isInternalGroup
is false
, the value of hasAccessToAllBuilds
is ignored and in the response hasAccessToAllBuilds
is set to nil. Of course I did also check that passing nil was fine on the real server.
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.
For a future PR, it might be nice to catch this and print a warning instead of silently ignoring the invalid parameter
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'll make a new 1-line PR
@getaaron I improved a bit the code, added |
@vincentisambart looks like there's still some CI failure @rogerluan have you seen this one? |
Looks like a flaky test... 🤔 |
c3a2f4a
to
5e3e587
Compare
I updated the commit date without changing the content to rerun the test. |
It seems after retry all tests passed. |
haha I had already retriggered CI from the re-run button, but I appreciate it 😊 glad the tests passed! I've never seen that flaky test before, but I'll keep an eye on it 👀 |
Thanks very much for the PR and all the follow up! |
Thank you for the review! |
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
When creating a beta group, the App Store Connect API let you specify if this new group has access to all existing builds or not, but fastlane did not let you specify it, setting it to
true
for internal groups, andfalse
for external ones.I wanted to be able to create internal groups that do not have access to existing builds.
Description
This change lets you explicitly specify if the new group has access to all existing builds or not. If you do not specify this new parameter, the behavior is the same as before.
Testing Steps