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

Explicit beta group access to all builds at creation #21478

Merged

Conversation

vincentisambart
Copy link
Contributor

@vincentisambart vincentisambart commented Aug 23, 2023

Checklist

  • I've run bundle exec rspec from the root directory to see all new and existing tests pass
  • I've followed the fastlane code style and run bundle exec rubocop -a to ensure the code style is valid
  • I see several green ci/circleci builds in the "All checks have passed" section of my PR (connect CircleCI to GitHub if not)
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary.

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, and false 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

@rogerluan
Copy link
Member

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!

@vincentisambart vincentisambart force-pushed the explicit-has_access_to_all_builds branch from d4ec0c6 to 0a5c52e Compare August 25, 2023 05:02
@vincentisambart
Copy link
Contributor Author

Rebase done 😁

Copy link
Collaborator

@getaaron getaaron left a 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 👍

@vincentisambart vincentisambart force-pushed the explicit-has_access_to_all_builds branch from 0a5c52e to c3a2f4a Compare August 28, 2023 01:25
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
Copy link
Contributor Author

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.

Copy link
Collaborator

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

Copy link
Contributor Author

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

@vincentisambart
Copy link
Contributor Author

@getaaron I improved a bit the code, added has_access_to_all_builds to the BetaGroup model, and added tests 😁

@getaaron
Copy link
Collaborator

@vincentisambart looks like there's still some CI failure

@rogerluan have you seen this one?

@rogerluan
Copy link
Member

Looks like a flaky test... 🤔

@vincentisambart vincentisambart force-pushed the explicit-has_access_to_all_builds branch from c3a2f4a to 5e3e587 Compare August 28, 2023 03:11
@vincentisambart
Copy link
Contributor Author

I updated the commit date without changing the content to rerun the test.

@vincentisambart
Copy link
Contributor Author

It seems after retry all tests passed.

@rogerluan
Copy link
Member

I updated the commit date without changing the content to rerun the test.

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 👀

@getaaron
Copy link
Collaborator

Thanks very much for the PR and all the follow up!

@getaaron getaaron merged commit ff74e77 into fastlane:master Aug 28, 2023
10 checks passed
@vincentisambart vincentisambart deleted the explicit-has_access_to_all_builds branch August 28, 2023 04:52
@vincentisambart
Copy link
Contributor Author

Thank you for the review!

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

3 participants