fix(cmake): properly fetch dev version by appending latest Falco tag, delta between master and tag, and hash #2292

FedeDP · 2022-11-15T15:03:04Z

What type of PR is this?

/kind bug

Any specific area of the project related to this PR?

/area build

What this PR does / why we need it:

Fix latest develoment Falco (packages and builds) having wrong 0.32.1... version.
describe can no more be used as tags are now made on release branches.

Which issue(s) this PR fixes:

Fixes #2270

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

FedeDP · 2022-11-15T15:24:24Z

It is working fine for gh actions; not working on CircleCI. I suspect git clone depth issues...

FedeDP · 2022-11-15T15:26:36Z

cmake/modules/GetGitRevisionDescription.cmake

@@ -86,44 +86,130 @@ function(get_git_head_revision _refspecvar _hashvar)
      PARENT_SCOPE)
 endfunction()

-function(git_describe _var)
+function(git_get_latest_tag _var)


Added 2 new functions: 1 to fetch latest tag in repository, the other to fetch commit delta between latest tag and HEAD.

FedeDP · 2022-11-15T15:26:59Z

userspace/falco/config_falco.h.in

@@ -16,8 +16,6 @@ limitations under the License.

 #pragma once

-#define FALCO_BRANCH "@FALCO_REF@"


Removed unused (always empty) macros.

FedeDP · 2022-11-15T15:28:23Z

cmake/modules/GetFalcoVersion.cmake

-    endif()
-    # Format FALCO_VERSION to be semver with prerelease and build part
-    string(REPLACE "-g" "+" FALCO_VERSION "${FALCO_VERSION}")
+      # Fetch current hash


New algorithm goes as follow:

fetch latest available tag in repo

fetch commits delta between tag and master (should be exactly the same as https://github.com/falcosecurity/falco/releases/latest "X commits to master since this release")

build the Falco version string

FedeDP · 2022-11-15T15:50:23Z

/cc @leogr

leogr · 2022-11-16T08:35:27Z

/assign
/milestone 0.34.0

FedeDP · 2022-11-16T08:56:14Z

The same fix should also be applied to libs ;)

FedeDP · 2022-12-05T14:29:46Z

A little bit better; but github actions are still seeing the correct version:

-- Falco version: 0.33.1-27+b40e044

While circleci is seeing:

-- Falco version: 0.33.1-4+2ea3398

Fun facts:

circleci is seeing the wrong number of commits since the tag (4), but it is seeing the correct commit hash: 2ea3398.
Instead gh actions are seeing the correct number of commits (27), but wrong commit hash: b40e044.
I am not sure why :/

… delta between master and tag, and hash. `describe` can no more be used as tags are now made on release branches. Signed-off-by: Federico Di Pierro <[email protected]>

…t in gh actions. See https://github.com/actions/checkout#checkout-pull-request-head-commit-instead-of-merge-commit. Signed-off-by: Federico Di Pierro <[email protected]>

FedeDP · 2022-12-05T15:01:17Z

Everything is fixed now.
We discovered that actions/checkout in gh actions is actually building a "fake" merge commit.
To disable the behavior, we needed to follow: https://github.com/actions/checkout#checkout-pull-request-head-commit-instead-of-merge-commit.
I will port the same fix to libs.

FedeDP · 2022-12-05T15:01:56Z

/cc @leogr @jasondellaluce

FedeDP · 2022-12-06T09:50:14Z

To disable the behavior, we needed to follow: https://github.com/actions/checkout#checkout-pull-request-head-commit-instead-of-merge-commit.
I will port the same fix to libs.

I am indeed not sure if we want this behavior (that is the same circleCI has) or we want to keep the merge commit one, to be notified about issues of current code rebased on top of master.

jasondellaluce

/approve

I like this one Fede! I think it makes sense for Falco to skip the merge commit in the GHA workflows, IMO it's more clear to have the same exact version in all artefacts.

poiana · 2022-12-09T11:01:02Z

LGTM label has been added.

Git tree hash: b04ec4d61424cce04a2db7c68125ba4f043198e0

jasondellaluce · 2022-12-09T11:01:11Z

/milestone 0.34.0

Andreagit97 · 2022-12-09T11:41:36Z

The only issue that we could have here is that by skipping the merge commit we could have the master broken after merging a PR, if there isn't a conflict poiana doesn't stop us but maybe the rebased code is broken :/

jasondellaluce · 2022-12-09T12:06:09Z

The only issue that we could have here is that by skipping the merge commit we could have the master broken after merging a PR, if there isn't a conflict poiana doesn't stop us but maybe the rebased code is broken :/

That's a good point, but we would also need to make sure that GHA merges the changes with a rebase, so that the end result is precisely what we would get if the PR gets merged. Not sure if this is supported though.

Andreagit97 · 2022-12-09T12:38:41Z

That's a good point, but we would also need to make sure that GHA merges the changes with a rebase

Yes exactly as far as I know it doesn't do that, we have the same issue I've described before if the user doesn't rebase the PR before merging it :/

Andreagit97 · 2022-12-09T12:40:49Z

we can also merge this and ask people to rebase before merging 🤔

poiana · 2022-12-12T10:11:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FedeDP, jasondellaluce, leogr

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [FedeDP,jasondellaluce,leogr]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

poiana added kind/bug release-note-none do-not-merge/work-in-progress area/build dco-signoff: yes size/L approved labels Nov 15, 2022

poiana requested review from Kaizhe and mstemm November 15, 2022 15:03

FedeDP force-pushed the fix/dev_version branch from 3fb04b6 to 753d0b1 Compare November 15, 2022 15:26

FedeDP commented Nov 15, 2022

View reviewed changes

poiana requested a review from leogr November 15, 2022 15:50

FedeDP force-pushed the fix/dev_version branch from 5d0167f to 753d0b1 Compare November 16, 2022 08:32

poiana assigned leogr Nov 16, 2022

poiana added this to the 0.34.0 milestone Nov 16, 2022

FedeDP force-pushed the fix/dev_version branch from 3a60945 to 753d0b1 Compare November 23, 2022 11:40

FedeDP force-pushed the fix/dev_version branch from 2ea3398 to 08d4669 Compare December 5, 2022 14:52

fix(cmake): properly fetch dev version by appending latest Falco tag,…

67d592e

… delta between master and tag, and hash. `describe` can no more be used as tags are now made on release branches. Signed-off-by: Federico Di Pierro <[email protected]>

FedeDP force-pushed the fix/dev_version branch from 08d4669 to 67d592e Compare December 5, 2022 14:55

chore(ci): properly checkout pull request HEAD instead of merge commi…

43e4d25

…t in gh actions. See https://github.com/actions/checkout#checkout-pull-request-head-commit-instead-of-merge-commit. Signed-off-by: Federico Di Pierro <[email protected]>

poiana requested a review from jasondellaluce December 5, 2022 15:01

FedeDP changed the title ~~wip: fix(cmake): properly fetch dev version by appending latest Falco tag, delta between master and tag, and hash~~ fix(cmake): properly fetch dev version by appending latest Falco tag, delta between master and tag, and hash Dec 5, 2022

poiana removed the do-not-merge/work-in-progress label Dec 5, 2022

jasondellaluce approved these changes Dec 9, 2022

View reviewed changes

poiana assigned jasondellaluce Dec 9, 2022

poiana added the lgtm label Dec 9, 2022

leogr approved these changes Dec 12, 2022

View reviewed changes

poiana merged commit 0c39776 into master Dec 12, 2022

FedeDP mentioned this pull request Feb 7, 2023

fix(cmake): fixed tag fetching fallback (that is indeed needed) #2409

Merged

FedeDP mentioned this pull request Mar 7, 2023

fix(cmake): fixed git dev versioning. falcosecurity/libs#946

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cmake): properly fetch dev version by appending latest Falco tag, delta between master and tag, and hash #2292

fix(cmake): properly fetch dev version by appending latest Falco tag, delta between master and tag, and hash #2292

FedeDP commented Nov 15, 2022

FedeDP commented Nov 15, 2022

FedeDP Nov 15, 2022

FedeDP Nov 15, 2022

FedeDP Nov 15, 2022 •

edited

Loading

FedeDP commented Nov 15, 2022

leogr commented Nov 16, 2022

FedeDP commented Nov 16, 2022

FedeDP commented Dec 5, 2022

FedeDP commented Dec 5, 2022

FedeDP commented Dec 5, 2022

FedeDP commented Dec 6, 2022

jasondellaluce left a comment

poiana commented Dec 9, 2022

jasondellaluce commented Dec 9, 2022

Andreagit97 commented Dec 9, 2022

jasondellaluce commented Dec 9, 2022

Andreagit97 commented Dec 9, 2022

Andreagit97 commented Dec 9, 2022

poiana commented Dec 12, 2022

		@@ -16,8 +16,6 @@ limitations under the License.

		#pragma once

		#define FALCO_BRANCH "@FALCO_REF@"

fix(cmake): properly fetch dev version by appending latest Falco tag, delta between master and tag, and hash #2292

fix(cmake): properly fetch dev version by appending latest Falco tag, delta between master and tag, and hash #2292

Conversation

FedeDP commented Nov 15, 2022

FedeDP commented Nov 15, 2022

FedeDP Nov 15, 2022

Choose a reason for hiding this comment

FedeDP Nov 15, 2022

Choose a reason for hiding this comment

FedeDP Nov 15, 2022 • edited Loading

Choose a reason for hiding this comment

FedeDP commented Nov 15, 2022

leogr commented Nov 16, 2022

FedeDP commented Nov 16, 2022

FedeDP commented Dec 5, 2022

FedeDP commented Dec 5, 2022

FedeDP commented Dec 5, 2022

FedeDP commented Dec 6, 2022

jasondellaluce left a comment

Choose a reason for hiding this comment

poiana commented Dec 9, 2022

jasondellaluce commented Dec 9, 2022

Andreagit97 commented Dec 9, 2022

jasondellaluce commented Dec 9, 2022

Andreagit97 commented Dec 9, 2022

Andreagit97 commented Dec 9, 2022

poiana commented Dec 12, 2022

FedeDP Nov 15, 2022 •

edited

Loading