-
-
Notifications
You must be signed in to change notification settings - Fork 268
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
workflows: Add github action checking emoji data matching with server. #918
base: main
Are you sure you want to change the base?
Conversation
5751356
to
3ffd9f1
Compare
b481654
to
c08ca8a
Compare
Wouldn't it be better if it's checked in every push and PR? If something breaks in a certain pr, it would be very difficult to identify it. Also, the main branch is supposed to be stable and we are all using it during development, so this goes against that. |
I second @Ezio-Sarthak's approach here. A CRON job might be better suited as these change infrequently as mentioned in the issue. We wouldn't have to run this as frequently as on every push and PR, also considering the frequency of that is highly variable. I presume the check is meant to be more general. Since the emoji list is auto-generated and not meant to be modified manually, the chance of something breaking in a PR should be significantly low. |
@Abhirup-99 I initially had a thought of doing so, but as mentioned by Neil in the issue, this (that is, change in emoji data) should be infrequent, and so a regular check could be enough. Also, since we have some workflows triggering already (specially codeql), having one more with every push would be a bloat to the repo. @prah23 Thanks for the clarification 👍 PS - The PR is open for reviews :) |
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.
@Ezio-Sarthak Thanks for looking into this 👍
The guts of this check seem good, but I do think we need to think about how the triggering and notification is going to occur.
I'm not sure how feasible it is, but sending the results to a Zulip topic may be the tidiest result here. If I understand the parallels, this is being worked on for zulip/zulip; I just posted in #automated testing>Unicode emoji sync to query this.
Given how minimal this is, we could limit this to trigger only on pushes to main on zulip/zulip-terminal. I'm not sure if we want this to fail CI due to this, but I'm not sure what other notification options we have that are straightforward.
If this runs on a timed schedule, I'm not sure where the result appears?
export DIFF_LEN=${#DIFF} | ||
if [[ ${DIFF_LEN} == 0 ]] ; then |
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.
Perhaps use --exit-code
with git diff
, and $?
?
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. I didn't know it before 😅
c08ca8a
to
9fc7034
Compare
@neiljp Thanks for the review! The notification of check on czo topic is a pretty awesome idea! |
2e613cd
to
7729ce8
Compare
7729ce8
to
0794127
Compare
0794127
to
bafbf72
Compare
c635220
to
b18488e
Compare
echo "Emojis up-to-date" | ||
else | ||
curl -X POST https://chat.zulip.org/api/v1/messages \ | ||
-u ${{ secrets.ZT_BOT_EMAIL }}:${{ secrets.ZT_BOT_API_KEY }} \ |
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.
From a skim of the secrets documents, we would likely want this to be in an environment variable rather than exposed in clear text.
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.
As of my knowledge, secrets
are default environment variable itself in GitHub actions.
However, It's good storing them as env
variables in any case 👍. This will tidy-up the relevant code wherever they are used. (currently double brackets seem visually messy).
--data-urlencode 'subject=Emoji sync with server' \ | ||
--data-urlencode 'content=Heads up :caution:! | ||
Emoji data needs to be updated.' |
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 easily change these later, but:
- I expect it'd be better to have a more general single topic for CI feedback, since we have everything in one stream.
- Re the content, we may wish to specify more that this originates from a server emoji update.
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.
- Re topic, maybe something like: "CI action feedback"?
- Re the content, the below message could be a potential description?:
"Heads up! Emoji sync with the server failed!
This originated from an update in server emoji. Kindly re-generateunicode_emojis.py
"
b18488e
to
bd857bd
Compare
2d4fae1
to
45607be
Compare
1b4714e
to
ffab889
Compare
ffab889
to
ce0807b
Compare
ce0807b
to
7d5ae35
Compare
The workflow uses CRON time syntax to schedule a check daily at a specific time. In case of a mismatch in fetched data and existing data, a message would be sent to CZO on the thread: '#zulip-terminal > CI action feedback' Currently the workflow would be triggered everyday at 17:00 UTC times. Fixes zulip#912.
7d5ae35
to
e849530
Compare
Heads up @Ezio-Sarthak, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the |
The workflow uses CRON time syntax to schedule the check daily at a specific time.
In case of a mismatch in fetched data and existing data, the check would fail with exit status: Emoji data not up-to-date.
[unrelated]: Corrects a couple of diffs :)
Fixes #912