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

ci: add simple build test workflow #696

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MichaIng
Copy link
Contributor

@MichaIng MichaIng commented Jan 13, 2025

as well as a dependabot config to update used actions in workflows.

When building from a fork, tags may not exist. Try to obtain latest tag from upstream via GitHub API in this case. Exit early if this fails as well, as DEB packages strictly require their version to start with an integer. For debugging reasons, error output is unmuted.

As we talked about it, as a start 🙂.

I took the if: logic of the workflow from our repos:

  • I personally want tests to run transparently on PR updates from fork branches, as well as on pushes to fork branches, which do not have a related PR.
  • Adding both triggers however leads to duplicates, when opening a PR with a branch from the own repo: tests run for the push, as well as for the PR update.
  • Using the push trigger only, runs the test on the fork repo (if it is from a fork), hence results do not show up and cannot be handled in the PR on the upstream/base repo.
  • Only solution I found is to add both triggers, but add this condition so that the PR-triggered tests run only, if the PR is from a fork.
  • As result:
    • the push triggered test runs always, hence on the own repo, if the branch is on the own repo, else on the fork. So people who work on their fork can also check and rerun tests on their own repo, like they run on my fork here now on every push: https://github.com/MichaIng/raspotify/actions
    • the PR triggered test runs only, if the PR has a fork as head. So test results can be seen and handled right within the PR. I am actually not sure if this happens as well, when the PR itself adds the test, hence if it does not exist on the base yet. So it might not be shown right here. Let's see ...
      EDIT: Oh nice, they do show up right here. So yeah, these are the "pull_request" triggered tests, intended to run only if the PR is from a fork, like in this case. When you open an internal PR, or push to any branch on your repo, a set of "push" triggered tests is supposed to run instead. And once the PR has been merged into the repos main branch, you can also manually trigger them for any branch from the actions panel, as I added the "workflow_dispatch" trigger as well: https://github.com/dtcooper/raspotify/actions

as well as a dependabot config to update used actions in workflows.

When building from a fork, tags may not exist. Try to obtain latest tag from upstream via GitHub API in this case. Exit early if this fails as well, as DEB packages strictly require their version to start with an integer. For debugging reasons, error output is unmuted.

Signed-off-by: MichaIng <[email protected]>
echo 'Obtaining latest Git repository tag for DEB package version ...'
RASPOTIFY_GIT_VER="$(git describe --tags "$(git rev-list --tags --max-count=1)" || :)"
if [ -z "$RASPOTIFY_GIT_VER" ]; then
echo 'Could not obtain latest tag from local repository. Obtaining it from upstream: https://api.github.com/repos/dtcooper/raspotify/tags'
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about starting the whole subroutine with git fetch --tags origin instead? This will ensure tags locally and not trigger a HTTP request to GitHub.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is not that tags are locally missing, but that the fork repo in fact does not have any tags, also not online. I could rephrase the log to "local/origin", to make that clear. It just makes sense for anyone who contributes to the project, forking the repo to make an edit, then open a PR upstream: forks do not contain any tags, unless they are created at the fork.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see. But is it really necessary to have the correct tag output in the build unless it is a release build? What is the value of having this tag?

Copy link
Contributor Author

@MichaIng MichaIng Jan 14, 2025

Choose a reason for hiding this comment

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

The tags are used for the version string or the DEB packages. Originally, without a tag, "unknown" is used, which is no valid version for DEB, as it strictly requires version strings to start with an integer. Of course we could "echo 1" or so, but I see no reason for DEB packages built on forks to have no proper version string, which compares well to upstream/distro/installed versions of the package, even if it is used for testing purposes only.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Very well, however I fear it will potentially silently ignore tags from a (faulty) local copy.
I want the checked out copy to be the source of truth for the version tag, not the upstream.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be best, but the only thing which contains the version string in a local copy/fork without tags is the CHANGELOG.md, which has not a very machine-readable format. I.e. the projects version solely relies on Git tags.

The version could be defined somewhere in the code, but then it needs to be additionaly maintained. The latest Git tag could still override the hardcoded version, to maintain current behavior for upstream releases.

.github/workflows/build-test.yml Show resolved Hide resolved
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