Conversation
Merge r2.8.3 develop into master
PR SummaryAdded functionality to source Firebase testers from both environment variables and local.properties, improving the flexibility of tester management in CI/CD pipeline. The system now merges testers from both sources, removes duplicates, and handles empty/null values gracefully. Changes
autogenerated by presubmit.ai |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (2)
Merge r2.8.3 develop into master
Files Processed (1)
- app/build.gradle (3 hunks)
Actionable Comments (0)
Skipped Comments (2)
-
app/build.gradle [63-67]
possible issue: "Potential issue with empty string handling in tester list processing"
-
app/build.gradle [61-62]
enhancement: "Consider validating environment variable format"
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- d6b26ba: test release
Files Processed (2)
- .github/workflows/release.yml (2 hunks)
- app/build.gradle (3 hunks)
Actionable Comments (1)
-
.github/workflows/release.yml [95-95]
best practice: "Commented out branch restriction could lead to unintended releases"
Skipped Comments (1)
-
app/build.gradle [61-67]
possible issue: "Potential issue with empty string handling in testers list"
| runs-on: ubuntu-latest | ||
| environment: production | ||
| if: github.ref == 'refs/heads/master' | ||
| # if: github.ref == 'refs/heads/master' |
There was a problem hiding this comment.
The commented out branch restriction (if: github.ref == 'refs/heads/master') removes a safety check that prevents releases from non-master branches. If this is intentional, consider adding a different branch restriction or documenting why the restriction was removed to prevent accidental releases.
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- 72af264: use testers var
Files Processed (1)
- .github/workflows/release.yml (4 hunks)
Actionable Comments (2)
-
.github/workflows/release.yml [80-81]
security: "Consider using secrets for sensitive data"
-
.github/workflows/release.yml [98-98]
possible issue: "Commented branch protection needs attention"
Skipped Comments (1)
-
.github/workflows/release.yml [8-8]
best practice: "Remove feature branch from workflow triggers"
| - name: Set Firebase testers from secret | ||
| run: echo "FIREBASE_TESTERS=${{ vars.FIREBASE_TESTERS }}" >> $GITHUB_ENV |
There was a problem hiding this comment.
Using GitHub Variables (vars) for Firebase testers might expose tester email addresses publicly in workflow logs. Consider using GitHub Secrets (secrets) instead of Variables if the tester list contains sensitive information like email addresses.
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- 6874def: clean up
Files Processed (1)
- .github/workflows/release.yml (2 hunks)
Actionable Comments (2)
-
.github/workflows/release.yml [79-80]
security: "Potential exposure of sensitive information in workflow logs"
-
.github/workflows/release.yml [148-149]
security: "Potential exposure of sensitive information in workflow logs"
Skipped Comments (0)
| - name: Set Firebase testers from secret | ||
| run: echo "FIREBASE_TESTERS=${{ vars.FIREBASE_TESTERS }}" >> $GITHUB_ENV |
There was a problem hiding this comment.
Using vars.FIREBASE_TESTERS could expose sensitive information in GitHub workflow logs. Consider using secrets.FIREBASE_TESTERS instead to better protect tester email addresses and other sensitive data. GitHub Secrets are automatically masked in logs while Variables are not.
| - name: Set Firebase testers from secret | ||
| run: echo "FIREBASE_TESTERS=${{ vars.FIREBASE_TESTERS }}" >> $GITHUB_ENV |
There was a problem hiding this comment.
Similar to the development workflow, using vars.FIREBASE_TESTERS in the release workflow could expose sensitive information in logs. Use secrets.FIREBASE_TESTERS instead to ensure tester information is properly masked in workflow logs.
nialexsan
changed the title
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- db5db91: Merge branch 'develop' into nialexsan/firebase-testers-env-var
Files Processed (1)
- app/build.gradle (3 hunks)
Actionable Comments (1)
-
app/build.gradle [63-67]
possible issue: "Potential issue with empty or malformed input handling"
Skipped Comments (0)
| def mergedTesters = ([localTesters, envTesters] | ||
| .findAll { it?.trim() } | ||
| .collectMany { it.split(",")*.trim() } | ||
| .toSet() // remove duplicates | ||
| .join(",")) |
There was a problem hiding this comment.
The current implementation might not handle edge cases well:
- If both sources are empty/null,
findAllwill filter them out butjoin(",")will still be called on an empty set - If the input contains malformed entries (e.g.,
",,"or" , "), the current logic will create empty entries
Consider adding input validation and sanitization:
def mergedTesters = ([localTesters, envTesters]
.findAll { it?.trim() }
.collectMany { it.split(',') }
.findAll { it.trim() } // Filter out empty entries after split
.toSet()
.findAll { it } // Additional safety check
.join(','))
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (1)
- 8d5f7c6: apply suggestion
Files Processed (1)
- app/build.gradle (3 hunks)
Actionable Comments (0)
Skipped Comments (2)
-
app/build.gradle [63-69]
possible issue: "Potential issue with list processing when input is malformed"
-
app/build.gradle [61-62]
best practice: "Missing validation for environment variable"
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (1)
- 5dd63e2: Merge branch 'develop' into nialexsan/firebase-testers-env-var
Files Processed (0)
Actionable Comments (0)
Skipped Comments (0)
3 participants
Related Issue
Closes #916
pick up testers from env vars instead of local.properties
Summary of Changes
Need Regression Testing
Risk Assessment
Additional Notes
Screenshots (if applicable)