-
Notifications
You must be signed in to change notification settings - Fork 891
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 CI workflow to check for incorrect catalog updates #6091
Conversation
a05acc0
to
02d3540
Compare
Codecov Report
@@ Coverage Diff @@
## main #6091 +/- ##
==========================================
- Coverage 81.52% 81.48% -0.04%
==========================================
Files 246 246
Lines 56781 56735 -46
Branches 12588 12569 -19
==========================================
- Hits 46288 46233 -55
- Misses 8092 8109 +17
+ Partials 2401 2393 -8 see 44 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
4e689b0
to
ee6d793
Compare
@jnidzwetzki, @erimatnor: please review this pull request.
|
scripts/check_latest-dev_updates.py
Outdated
# also need a function to separate comments from actual commands | ||
def remove_comments(lines): | ||
code_lines = [] | ||
for line in lines: |
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.
More idiomatic I believe:
code_lines = (line for line in lines if not line.startswith("--"))
return "\n".join(code_lines)
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 @wrobell
ref: ${{ github.event.pull_request.head.sha }} | ||
fetch-depth: 0 | ||
- name: Check if latest-dev was modified | ||
shell: bash --norc --noprofile {0} |
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.
What is the reason for these parameters? Could you add a comment?
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.
tbh I'm not sure why, I see we specify these in other workflows but I don’t know the reason, so let’s test without them and find out if they’re needed
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 seems bash {0} is enough and -eo pipefail {0}
is what we need to avoid in this workflow which is by default set if we simply use the bash option (https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idstepsshell)
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.
Looks reasonable. Added a few minor comments.
cfdaf90
to
1ebb4fb
Compare
5f72946
to
ba6b5c4
Compare
ec24cb0
to
9150d4f
Compare
When adding or dropping columns to timescaledb catalog tables, the right way to do this is to drop and recreate the table with the desired definition, instead of doing ALTER TABLE .. ADD/DROP COLUMN. This is required to ensure consistent attribute numbers across versions. This workflow will fail with an error if it detects such an incorrect modification of a catalog table. Fixes timescale#6049
9150d4f
to
2c62b33
Compare
2c62b33
to
38754f1
Compare
As discussed at standup today, two more checks need to be added:
|
Have you considered using proper sql parser instead of "grepping" the text. Using proper sql parser would be more robust. pgspot uses https://pypi.org/project/pglast/ |
run: | | ||
file="sql/updates/latest-dev.sql" | ||
updated=0 | ||
diff=$(gh pr view $PR_NUMBER --json files --jq '.files[] | select(.path == "sql/updates/latest-dev.sql") | .path') | ||
if [[ ${diff} != "" ]]; then | ||
updated=1 | ||
fi | ||
echo "latestdev_updated=$updated" >> $GITHUB_OUTPUT |
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 we want to run this always. This should be pretty quick check and not running it when the script itself is adjusted seems suboptimal. In addition we also want to run this on reverse-dev.sql
I will update this PR or create a new one to use pglast instead of regular expressions, working on the changes here https://github.com/konskov/timescaledb/tree/ci_check_alternative_pglast |
closing in favor of #6130 |
When adding or dropping columns to timescaledb catalog tables, the
right way to do this is to drop and recreate the table with the desired
definition, instead of doing ALTER TABLE .. ADD/DROP COLUMN. This is
required to ensure consistent attribute numbers across versions.
This workflow will fail with an error if it detects such an incorrect
modification of a catalog table.
Fixes #6049