-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Add FXIOS-9487 - Remote settings config auto fetch script for password rules #21023
Add FXIOS-9487 - Remote settings config auto fetch script for password rules #21023
Conversation
@issammani I am tagging you as you did some work around fetching data using python script from HG for autofill |
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.
This looks great. Thanks @nbhasin2.
Tried locally and it works as expected when the json file changed / not changed. I left some minor nits.
firefox-ios/Client/Assets/RemoteSettingsData/Update_Remote_Settings.py
Outdated
Show resolved
Hide resolved
|
||
if not changes_detected: | ||
print("No changes detected in any rules.") | ||
|
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 won't break the code since we check if the directory exists before creating it, but maybe we should remove the directory after we are done ? Something like shutil.rmtree(GITHUB_ACTIONS_TMP_PATH)
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.
true, I left it here as I wasn't sure what to do here 🤷🏼
One question that I had was since its run on github actions, does it spin a new VM everytime an action is run or all share the same VM
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.
Good question. So looking at the docs, github actions doesn't cache that directory between runs. And since the files in there won't be committed and not in the final PR it doesn't really matter if we remove it or not 😄
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.
We can leave it be then for any logs during that run!
"name": "Password Rules", | ||
"url": "https://firefox.settings.services.mozilla.com/v1/buckets/main/collections/password-rules/records", | ||
"file": "./firefox-ios/Client/Assets/RemoteSettingsData/RemotePasswordRules.json" | ||
} |
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.
Making it ready for review, once this gets merged I will start the process of adding more files for upcoming remote settings work. So think of this as the initial step
Once @isabelrios gives the r+ I will merge this in 🤞🏼 |
- name: Commit and push changes | ||
run: | | ||
git diff | ||
git diff --quiet || (git add . && git commit -m "Update remote settings with latest rules" && git push) |
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.
If I got it right you are creating a temp file when comparing the changes.. git git add . it may be added... I always try to be specific about the files that I would expect to be changed and commited in the PR to save time not having to fix the automatic PR
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.
Oh you mean, make it explicit?
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.
Yes, if that makes sense.. something like: https://github.com/nbhasin2/firefox-ios-1/blob/0ac1fe4aa1ac3663fc1a98ee09f8853066e5ff99/.github/workflows/check-rust-component-dependency.yml#L72
- name: Send Slack notification if GitHub Action fails | ||
if: '!cancelled()' | ||
id: slack | ||
uses: slackapi/slack |
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 think this needs to be like: slackapi/[email protected]
, I see an error in your for for this github action
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.
Also, not sure if just with that there will be a notification, usually there are some fields to be filled in in this step.. we can try to trigger the action from your fork and see
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.
Ah since I was basing it off
https://github.com/mozilla-mobile/firefox-ios/blob/main/.github/workflows/firefox-ios-update_credential_provider_script.yml
and tag you again for another review :)
Good catch @isabelrios
This PR has been automatically marked as stale. Please leave any comment to keep this PR opened. It will be closed automatically if no further update occurs in the next 7 days. Thank you for your contributions! |
0ac1fe4
to
1d7cdca
Compare
env: | ||
JOB_STATUS: ${{ job.status == 'success' && ':white_check_mark:' || job.status == 'failure' && ':x:' }} | ||
JOB_STATUS_COLOR: ${{ job.status == 'success' && '#36a64f' || job.status == 'failure' && '#FF0000' }} | ||
SLACK_WEBHOOK_URL: ${{ secrets.SLACK_WEBHOOK_URL }} | ||
SLACK_WEBHOOK_TYPE: INCOMING_WEBHOOK | ||
with: | ||
payload-file-path: "./test-fixtures/ci/slack-notification-payload-remote-settings-fetch.json" |
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.
@isabelrios added payload, could you review please 🙏🏼
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.
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.
Thanks Isabel, we can leave it there for now. Maybe once I add more items we can look into sending it to its own area.
- name: Commit and push changes | ||
run: | | ||
git diff | ||
git diff --quiet || (git add firefox-ios/Client/Assets/RemoteSettingsData/RemotePasswordRules.json && git commit -m "Update remote settings with latest rules" && git push) |
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.
@isabelrios also added specific json file location, tagging you to double check
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.
You would need to remove the latest part: && git commit -m "Update remote settings with latest rules" && git push
The github action fails to do the push directly, I don't think we want that push to happen without review..
If you remove that part, there is the next step to create the PR with the changes to review and then merge
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.
@isabelrios let me DM you as I am wondering why do I even need the commit part considering we are doing the create pull request
Generated by 🚫 Danger Swift against 04ed227 |
Sorry, there is one more change to make or the github action will consistently fail. Please see my comment to remove part of the git diff statement. Thanks! |
📜 Tickets
Jira ticket
Github issue
💡 Description
This script is to fetch remote settings on Tuesdays and Thursdays automatically and create necessary PRs if there are any changes.
Ref: https://github.com/issammani/firefox-ios/blob/main/.github/workflows/firefox-ios-update_credential_provider_script.yml
📝 Checklist
You have to check all boxes before merging
@Mergifyio backport release/v120
)