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

PX4 Metadata upload to S3 isn't being separated to appropriate releases #22383

Open
junwoo091400 opened this issue Nov 16, 2023 · 5 comments
Open

Comments

@junwoo091400
Copy link
Contributor

Describe the bug

Currently, metadata being uploaded to S3 bucket is not being separated into different folders (e.g. stable, beta, master, release/1.x) as it should.

For detailed description visit this closed PR: #21868

To Reproduce

Check the "Deploy metadata" github action runs for v1.14 release for example: https://github.com/PX4/PX4-Autopilot/actions/workflows/deploy_all.yml?query=branch%3Arelease%2F1.14

All the metadata can be seen being written to "Firmware/master" directory. For example: http://px4-travis.s3.amazonaws.com/Firmware/master/px4_fmu-v5_default/actuators.json

Expected behavior

The metadata should be appropriately separated into different release version directories like:

  • Firmware/master/
  • Firmware/release-1.14/ (or Firmware/release/1.14, etc)
  • Firmware/release-1.13/, etc.

Fix for this should be to make "px_update_git_header.py" script appropriately set the "version" environment variable in Github actions, as currently it's just being set as "master".

@bkueng I am guessing that currently QGC isn't fetching release version specific metadata from the S3, but we would want it to do that right? (along with bugfix for this)

Screenshot / Media

No response

Flight Log

None

Software Version

main

Flight controller

None

Vehicle type

None

How are the different components wired up (including port information)

No response

Additional context

This fix is needed to make the documentation PX4/PX4-user_guide#2284 consistent

@bkueng
Copy link
Member

bkueng commented Nov 16, 2023

@junwoo091400 can you look into this? Looks like this is not correctly set:

#define PX4_GIT_TAG_OR_BRANCH_NAME "{tag_or_branch}" // special variable: git tag, release or master branch

Possibly because github does not checkout the branch, but directly the commit.

@bkueng I am guessing that currently QGC isn't fetching release version specific metadata from the S3, but we would want it to do that right? (along with bugfix for this)

Yes, if the above is correct then the metadata url's will also be correct.

@junwoo091400
Copy link
Contributor Author

junwoo091400 commented Nov 16, 2023

Possibly because github does not checkout the branch, but directly the commit.

image

https://github.com/PX4/PX4-Autopilot/actions/runs/6889003852/job/18739109388

@bkueng It does seem like it is checking out a commit directly, hence the branch name parsing functionality is broken. Now trying to figure out how to checkout branch directly instead of the commit with the actions/checkout.

On a side note though, do we really want any commit checked out to report back "master" as version tag? Is there no way of figuring out the version within the source code itself, other than branch name?

@junwoo091400
Copy link
Contributor Author

image

It seems like indeed checking out the commit itself produces the "PX4_GIT_TAG_OR_BRANCH_NAME" as 'master', when executing 'src\lib\version\px_update_git_header.py' directly.

And so I think the behavior should be following instead for "Git tag or branch name" varaible:

  1. If on exact git tag, use that tag (e.g. v1.14.0-beta2)
  2. If not, but on release branch (release/*), use that release name (e.g. release-1.14)
  3. If not, but on master or main branch, use "master" instead
  4. If not (on detached commit, or non-release/master/main branch), set to ""

This way, I think we can support cases of:

  1. Having main/master branch updates to metadata being updated to "master" directory
  2. Having only exact commits that were tagged with RC/beta/minor releases get metadata built and uploaded to appropriate directories like "release-1.14" (should already be the case, but checking v1.14.0 tagged commit's CI run shows that it still outputs "master", although locally I can reproduce that it outputs "v1.14.0" even when checking the commit itself locally)
  3. For known release/* branches, have metadata uploaded to appropriate directories like "release-1.14"
  4. For the rest case, don't upload the metadata, as it is likely not relevant for releases.

I would say that then we should do the following:

  1. Fix the "px_update_git_header.py" to embody the logic mentioned above
  2. Fix detection for updates in "release/*" branches via editing "deploy_all.yml" script to override the tag or branch name thing, when we know that we are on release branch
  3. Add mechanism to not upload anything when the returned tag or branch name is ""

@bkueng thoughts on this?

@junwoo091400
Copy link
Contributor Author

One update, I noticed the case where the tagged commits' github actions still outputting "master". I think that happened because the 'tagging' happened *after that commit was pushed to the release branch, as there were some times spent on release notes, etc.

So, I have tried verifying that hypothesis by simply re-running the commit tagged with v1.14.0, and it does indeed now update the metadata into the v1.14.0 directory as we intend to!

Example file is now here: https://px4-travis.s3.amazonaws.com/Firmware/v1.14.0/px4_fmu-v5x_default/actuators.json

Of course this workaround shouldn't be the solution, so that means adding the 2nd solution mentioned above will fix this case.

@bkueng
Copy link
Member

bkueng commented Nov 17, 2023

Thanks for looking into it. So there's 2 issues, both related how github handles checkouts/tagging:

If not (on detached commit, or non-release/master/main branch), set to ""

This means there would be no metadata when building (locally) on another branch

For the rest case, don't upload the metadata, as it is likely not relevant for releases.

That's already the case with the CI branch filtering.

@github-actions github-actions bot added the stale label Dec 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants