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

just a CI test #5913

Closed
wants to merge 16 commits into from
Closed

just a CI test #5913

wants to merge 16 commits into from

Conversation

m-schmoock
Copy link
Collaborator

Just a test PR, trying to debug some CI issues...

Print the schema diff when there are missing 'added'/'deprecated' fields.
This saves some time when cleaning up code before using CI.

Changelog-None
This fixes the CI errors when doing `make check-source`
steps 'schema-added-check' and 'schema-removed-check'.
These errors prevented CI from performing these steps correctly.

I changed it so that they do a `git fetch origin` at first and do
the `git diff` against 'origin/master...HEAD' instead of just master.
Also fixed a bug in the script that was missing $$master in the same line.

```
fatal: ambiguous argument 'main': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
```
@cdecker
Copy link
Member

cdecker commented Jan 20, 2023

Are you sure Github just renamed the branch without prompting us? That'd be a very surprising move and definitely cause a lot of trouble. I just checked out this repo and the main branch is still called master.

@m-schmoock
Copy link
Collaborator Author

m-schmoock commented Jan 20, 2023

@cdecker Yes, the make script CI is calling is definitively not working correctly (anymore?)
Current master source results in the following CI output when doing make check-source

fatal: ambiguous argument 'main': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'

Currently fiddling around to fix that.
What I simply do is a git fetch origin/master and do the diffs against that. This way we don't need the if/else main/master foo.
I'll keep you updated once I have something working...

@cdecker
Copy link
Member

cdecker commented Jan 20, 2023

That usually means that there is both a branch main and a file/directory main and git can't uniquely identify which you meant. Let me check that :-)

@cdecker
Copy link
Member

cdecker commented Jan 20, 2023

Ok, that theory falls through quickly: no main branch nor a main file.

doc/Makefile Outdated
@@ -209,11 +209,11 @@ doc/index.rst: $(MANPAGES:=.md)

# So GitHub renamed master to main. This is painful.
schema-added-check:
@if ! git describe master >/dev/null 2>&1; then MASTER=main; else MASTER=master; fi; if git diff $$MASTER doc/schemas | grep -q '^+.*{' && ! git diff master doc/schemas | grep -q '^+.*"added"'; then echo 'New schema fields must have "added": "vNEXTVERSION"' >&2; exit 1; fi
@git fetch origin master ; if git diff origin/master...HEAD -- doc/schemas | grep -q '^+.*{' && ! git diff origin/master...HEAD -- doc/schemas | grep -q '^+.*"added"'; then git diff origin/master...HEAD -- doc/schemas ; echo 'New schema fields must have "added": "vNEXTVERSION"' >&2; exit 1; fi
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

git fetch in a script will cause people headaches if they have a passphrase or a hardware key holding the SSH keys.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, can't do it that way... :/

@m-schmoock
Copy link
Collaborator Author

@cdecker looks like GitHub CI is not checking out the repo with any refs except the PRs branch itself. I think the master/main naming issue doesn't apply for us, as our repo has 'master' branch and it will likely stay that way. The political correct 'main' branch is only for new projects. So we can remove that main/master if/else from our script.

I will try to change it in a way that only when it detects that it is running on GitHub actions CI it will do the 'fetch origin master'.

@m-schmoock m-schmoock closed this Jan 20, 2023
@m-schmoock m-schmoock deleted the foobar branch January 20, 2023 22:54
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.

2 participants