Add transformation test make target and docs #1722

braydonk · 2024-05-23T15:11:03Z

Description

This PR adds make targets for transformation tests, as well as usage docs.

Some other things were fixed incidentally:

Running the precommit_update target yielded numerous files missing license headers
Fixed warnings in transformation_test.go

Related issue

These updates will feed into updating the otelopscol version.

How has this been tested?

All make targets have been run.

Checklist:

Unit tests
- Unit tests do not apply.
- Unit tests have been added/modified and passed for this PR.
Integration tests
- Integration tests do not apply.
- Integration tests have been added/modified and passed for this PR.
Documentation
- This PR introduces no user visible changes.
- This PR introduces user visible changes and the corresponding documentation change has been made.
Minor version bump
- This PR introduces no new features.
- This PR introduces new features, and there is a separate PR to bump the minor version since the last release already.
- This PR bumps the version.

This PR adds make targets for transformation tests, as well as usage docs. Some other things were fixed incidentally: * Running the `precommit_update` target yielded numerous files missing license headers * Fixed warnings in `transformation_test.go` Signed-off-by: braydonk <[email protected]>

builds/fluent_bit_local.sh

.gitignore

builds/fluent_bit_local.sh

igorpeshansky · 2024-05-23T16:03:09Z

dev-docs/makefile.md

+
+`tasks.mak` is provided without any guarantees or warranty on its targets. It is meant purely for developer convenience, and it is advised not to make any dependency on the targets since they are subject to change at any time.


[Optional] I wonder if we should be calling it developer_tasks.mak or something…

I originally chose tasks.mak just cause it was shorter, and at first it seemed like we'd need to always type in -f tasks.mak so making it longer would have been a pain. We could change it now, though it really only exists to then be immediately symlinked to when you set up your environment.

dev-docs/transformation-tests.md

tasks.mak

transformation_test/transformation_test.go

I think this makes more sense, and also fixes a typo I had here previously that caused the tests to fail.

This reverts commit dbb99ab.

builds/fluent_bit.sh

igorpeshansky · 2024-05-23T20:59:34Z

builds/fluent_bit.sh

-  -DFLB_CONFIG_YAML=OFF
-make -j8
-make DESTDIR="$DESTDIR" install
+if [[ "$DESTDIR" = *"dist" ]]; then


I wonder — is there any harm in actually doing a full install into ${DESTDIR} and pointing the tests to the installation path within that? That way we can fully reuse the existing build script, right?
As-is, using a DESTDIR pattern to indicate local builds seems fragile to me. I'd almost rather add a new variable (e.g., LOCAL_BUILD, or BINARY_ONLY, or some such) and use that…

Honestly we could do the install, the main reason I didn't is because it's super annoying how many folders it creates when we could just copy the binary

Added LOCAL_ONLY variable in f1d154e

IMO, those additional folders are a small price to pay for removing the additional complexity in the build scripts. But let's see how non-invasive we can make this with LOCAL_ONLY first…

builds/fluent_bit.sh

builds/otel.sh

igorpeshansky · 2024-05-24T15:52:28Z

builds/fluent_bit.sh

-DFLB_CONFIG_YAML=OFF
-END
+CMAKE_ARGS=(
+  "-DCMAKE_BUILD_TYPE=RelWithDebInfo"


[Optional] These quotes are also superfluous (but harmless).

On second thought, these flags didn't use to have quotes on the command line, so it may make sense to not use quotes in the array as well… Except, maybe, for CMAKE_INSTALL_PREFIX, which could become -DCMAKE_INSTALL_PREFIX="$fluent_bit_dir"… WDYT?

igorpeshansky · 2024-05-24T15:57:09Z

builds/fluent_bit.sh

-  -DFLB_CONFIG_YAML=OFF
-make -j8
-make DESTDIR="$DESTDIR" install
+if [[ "$DESTDIR" = *"dist" ]]; then


IMO, those additional folders are a small price to pay for removing the additional complexity in the build scripts. But let's see how non-invasive we can make this with LOCAL_ONLY first…

builds/fluent_bit.sh

igorpeshansky · 2024-05-24T16:09:55Z

builds/fluent_bit.sh


 cd submodules/fluent-bit
 mkdir -p build
 cd build
-if [[ "$DESTDIR" = *"dist" ]]; then
-  cmake .. $CMAKE_ARGS
+
+if [ "$LOCAL_ONLY" = "true" ]; then
+  # If this is a local build, we can just build and copy the binary.
+  cmake .. ${CMAKE_ARGS[@]}
  make -j8
-  mv ./bin/fluent-bit $DESTDIR
+  cp ./bin/fluent-bit $DESTDIR
+
+# Otherwise, we do a full install.
 else
+  fluent_bit_dir=/opt/google-cloud-ops-agent/subagents/fluent-bit
  # CMAKE_INSTALL_PREFIX here will cause the binary to be put at
  # /usr/lib/google-cloud-ops-agent/bin/fluent-bit
-  # Additionally, -DFLB_SHARED_LIB=OFF skips building libfluent-bit.so
-  cmake .. -DCMAKE_INSTALL_PREFIX=$fluent_bit_dir $CMAKE_ARGS
+  cmake .. -DCMAKE_INSTALL_PREFIX=$fluent_bit_dir ${CMAKE_ARGS[@]}
  make -j8


We can factor out the additional CMAKE_ARGS and the install step into the non-local case…

if ! [ "$LOCAL_ONLY" = "true" ]; then fluent_bit_dir=/opt/google-cloud-ops-agent/subagents/fluent-bit CMAKE_ARGS=( # CMAKE_INSTALL_PREFIX here will cause the binary to be put at # /usr/lib/google-cloud-ops-agent/bin/fluent-bit "-DCMAKE_INSTALL_PREFIX=$fluent_bit_dir" "${CMAKE_ARGS[@]}" ) fi cd submodules/fluent-bit mkdir -p build cd build cmake .. "${CMAKE_ARGS[@]}" make -j8 # If this is a local build, we can just build and copy the binary. if [ "$LOCAL_ONLY" = "true" ]; then cp ./bin/fluent-bit "$DESTDIR" # Otherwise, we do a full install. else

Obsoleted by 78fbf64 which changes the script to always function with a full install.

[Optional] Heh, we could now revert back to the original inline args, unless you want to keep the array…

There's actually no reason for this script to change at all anymore. Reverted it to its state in the main branch in 4ed4297 (left the license header added though)

builds/fluent_bit.sh

dev-docs/transformation-tests.md

tasks.mak

igorpeshansky · 2024-05-24T16:58:11Z

tasks.mak

+test_confgenerator:
+	go test ./confgenerator $(if $(UPDATE),-update,)
+
+.PHONY: test_confgenerator_update
 test_confgenerator_update:


[Optional] Would target-specific variables work in this case, e.g.:

test_confgenerator_update: UPDATE=true test_confgenerator test_metadata_update: UPDATE=true test_metadata … transformation_test_update: UPDATE=true transformation_test

? We might need := instead of =, but it ought to work…

I couldn't get this to work. The only way UPDATE=true was properly passed to the next target is the way I have it right now.

Ah, I see what I did wrong — the variable assignment has to be a separate dependency. This works:

test_confgenerator_update: UPDATE=true test_confgenerator_update: test_confgenerator test_metadata_update: UPDATE=true test_metadata_update: test_metadata … transformation_test_update: UPDATE=true transformation_test_update: transformation_test

$ cat Makefile t: echo "[$(UPDATE)]" u: UPDATE=true u: t v: UPDATE=false v: t $ make t echo "[]" [] $ make u echo "[true]" [true] $ make v echo "[false]" [false] $

But up to you…

Done in d0194ea

igorpeshansky · 2024-05-29T20:39:07Z

builds/otel.sh

@@ -15,17 +15,17 @@

 set -x -e

-ROOTDIR="$1"
+DESTDIR="$1"


The quotes around $1 are superfluous (and didn't use to be there) — want to remove them for symmetry with fluent_bit.sh?

igorpeshansky · 2024-05-29T20:44:03Z

builds/otel.sh

+DESTDIR="$1"
 otel_dir=/opt/google-cloud-ops-agent/subagents/opentelemetry-collector
-DESTDIR="${ROOTDIR}${otel_dir}"
+DESTDIR="${DESTDIR}${otel_dir}"
+
+mkdir -p $DESTDIR


Might be a good idea to have a separate name for the otel binary directory, and create the original $DESTDIR separately, i.e.:

DESTDIR=$1 mkdir -p "$DESTDIR" OTELDIR="${DESTDIR}${otel_dir}" mkdir -p "$OTELDIR"

I changed it anyway in 0800d03. The first assignment of DESTDIR was completely redundant. The DESTDIR is never used separately. The mkdir -p $DESTDIR that's in there now should work fine.

igorpeshansky · 2024-05-29T20:48:56Z

builds/fluent_bit.sh

 CMAKE_ARGS=(
+  # CMAKE_INSTALL_PREFIX here will cause the binary to be put at
+  # /usr/lib/google-cloud-ops-agent/bin/fluent-bit


Just realized that this value is no longer accurate, is it?

Obsoleted by 4ed4297

igorpeshansky · 2024-05-29T20:50:23Z

builds/fluent_bit.sh

-DFLB_CONFIG_YAML=OFF
-END
+CMAKE_ARGS=(
+  "-DCMAKE_BUILD_TYPE=RelWithDebInfo"


On second thought, these flags didn't use to have quotes on the command line, so it may make sense to not use quotes in the array as well… Except, maybe, for CMAKE_INSTALL_PREFIX, which could become -DCMAKE_INSTALL_PREFIX="$fluent_bit_dir"… WDYT?

igorpeshansky · 2024-05-29T20:56:16Z

builds/fluent_bit.sh


 cd submodules/fluent-bit
 mkdir -p build
 cd build
-if [[ "$DESTDIR" = *"dist" ]]; then
-  cmake .. $CMAKE_ARGS
+
+if [ "$LOCAL_ONLY" = "true" ]; then
+  # If this is a local build, we can just build and copy the binary.
+  cmake .. ${CMAKE_ARGS[@]}
  make -j8
-  mv ./bin/fluent-bit $DESTDIR
+  cp ./bin/fluent-bit $DESTDIR
+
+# Otherwise, we do a full install.
 else
+  fluent_bit_dir=/opt/google-cloud-ops-agent/subagents/fluent-bit
  # CMAKE_INSTALL_PREFIX here will cause the binary to be put at
  # /usr/lib/google-cloud-ops-agent/bin/fluent-bit
-  # Additionally, -DFLB_SHARED_LIB=OFF skips building libfluent-bit.so
-  cmake .. -DCMAKE_INSTALL_PREFIX=$fluent_bit_dir $CMAKE_ARGS
+  cmake .. -DCMAKE_INSTALL_PREFIX=$fluent_bit_dir ${CMAKE_ARGS[@]}
  make -j8


[Optional] Heh, we could now revert back to the original inline args, unless you want to keep the array…

builds/otel.sh

dev-docs/transformation-tests.md

igorpeshansky · 2024-05-29T21:02:24Z

tasks.mak

+	FLB=$(PWD)/dist/opt/google-cloud-ops-agent/subagents/fluent-bit/bin/fluent-bit \
+	OTELOPSCOL=$(PWD)/dist/opt/google-cloud-ops-agent/subagents/opentelemetry-collector/otelopscol \


[Optional] Let's use $${PWD} (bash variable quoting) instead of $(PWD) (makefile quoting)?

Why wouldn't we want to use makefile quoting in the makefile?

Depends on whether you want the directory the make command was invoked from ($(PWD), which is a make variable), or the current directory of the shell that make is running the current set of commands in ($${PWD}, which is a shell variable). They might be the same in this case, I suppose…

In this case, I do want the working directory of make and not the working directory of the shell, although I do not change the working directory of the shell during this target.

igorpeshansky · 2024-05-29T21:10:39Z

tasks.mak

+test_confgenerator:
+	go test ./confgenerator $(if $(UPDATE),-update,)
+
+.PHONY: test_confgenerator_update
 test_confgenerator_update:


Ah, I see what I did wrong — the variable assignment has to be a separate dependency. This works:

test_confgenerator_update: UPDATE=true test_confgenerator_update: test_confgenerator test_metadata_update: UPDATE=true test_metadata_update: test_metadata … transformation_test_update: UPDATE=true transformation_test_update: transformation_test

$ cat Makefile t: echo "[$(UPDATE)]" u: UPDATE=true u: t v: UPDATE=false v: t $ make t echo "[]" [] $ make u echo "[true]" [true] $ make v echo "[false]" [false] $

But up to you…

…in makefile

dev-docs/transformation-tests.md

tasks.mak

igorpeshansky · 2024-06-04T22:41:03Z

tasks.mak

+	FLB=$(PWD)/dist/opt/google-cloud-ops-agent/subagents/fluent-bit/bin/fluent-bit \
+	OTELOPSCOL=$(PWD)/dist/opt/google-cloud-ops-agent/subagents/opentelemetry-collector/otelopscol \


Depends on whether you want the directory the make command was invoked from ($(PWD), which is a make variable), or the current directory of the shell that make is running the current set of commands in ($${PWD}, which is a shell variable). They might be the same in this case, I suppose…

braydonk · 2024-11-14T23:55:20Z

Something that I want to do over FixIt is to clean up the Makefile and make it better since I'm much better at writing make than I was when I first made the Makefile. Also, our update to goccy/go-yaml is dependent on making some transformation_test updates, which are really hard to do right now without these make targets. So provided CI passes, I am going to merge this and address the feedback when I do the cleanup during the FixIt.

braydonk requested review from a team, XuechunHou and igorpeshansky and removed request for a team and XuechunHou May 23, 2024 15:11

jefferbrecht reviewed May 23, 2024

View reviewed changes

builds/fluent_bit_local.sh Outdated Show resolved Hide resolved

jefferbrecht reviewed May 23, 2024

View reviewed changes

builds/fluent_bit_local.sh Outdated Show resolved Hide resolved

jefferbrecht reviewed May 23, 2024

View reviewed changes

builds/fluent_bit_local.sh Outdated Show resolved Hide resolved

igorpeshansky reviewed May 23, 2024

View reviewed changes

braydonk added 8 commits May 23, 2024 17:34

remove unintended function

33a760b

use target dependencies instead of wildcard

73e5155

change error name from cmdErr to err

79127e9

I think this makes more sense, and also fixes a typo I had here previously that caused the tests to fail.

factor out cmake arguments into a separate file

dbb99ab

Just kidding that doesn't work

91c0397

This reverts commit dbb99ab.

consolidate build scripts

cfcc68f

reword gitignore comments

a4b98df

reword transformation-tests.md intro paragraph

65cfc75

igorpeshansky reviewed May 23, 2024

View reviewed changes

braydonk added 6 commits May 24, 2024 13:18

add LOCAL_ONLY variable support

f1d154e

remove unnecessary var

e87fa1e

move shared lib comment

0572082

changed CMAKE_ARGS to an array

6d2aeac

properly quote ldflag

34654d8

set me free from bash hell

e1164e0

igorpeshansky reviewed May 24, 2024

View reviewed changes

braydonk added 2 commits May 27, 2024 13:11

Change build scripts to full install

78fbf64

fix paths for new full install builds, update some out of date bits

b94c571

braydonk added 2 commits May 29, 2024 20:31

change P variable to FILE

ea76294

fix outdated comment

c66ca2d

igorpeshansky reviewed May 29, 2024

View reviewed changes

braydonk added 4 commits May 31, 2024 13:55

revert fluent bit build script back to the way it was

4ed4297

use target-specific variables for update targets

d0194ea

remove redundant DESTDIR assignment

0800d03

clarify java skip in transformation test docs and add way to do java …

0db4c79

…in makefile

igorpeshansky reviewed Jun 5, 2024

View reviewed changes

braydonk added 2 commits November 14, 2024 23:43

merge master

eb5d8ef

update some goldens

2853765

fix dev-doc grammar

6e0c84a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add transformation test make target and docs #1722

Add transformation test make target and docs #1722

braydonk commented May 23, 2024

igorpeshansky May 23, 2024

braydonk May 23, 2024

igorpeshansky May 23, 2024

braydonk May 24, 2024 •

edited

Loading

braydonk May 24, 2024

igorpeshansky May 24, 2024

igorpeshansky May 24, 2024

igorpeshansky May 29, 2024

igorpeshansky May 24, 2024

igorpeshansky May 24, 2024

braydonk May 29, 2024

igorpeshansky May 29, 2024

braydonk May 31, 2024

igorpeshansky May 24, 2024

braydonk May 29, 2024

igorpeshansky May 29, 2024

braydonk May 31, 2024

igorpeshansky May 29, 2024

igorpeshansky May 29, 2024

braydonk May 31, 2024

igorpeshansky May 29, 2024

braydonk May 31, 2024

igorpeshansky May 29, 2024

igorpeshansky May 29, 2024

igorpeshansky May 29, 2024

braydonk May 31, 2024

igorpeshansky Jun 4, 2024

braydonk Nov 15, 2024

igorpeshansky May 29, 2024

igorpeshansky Jun 4, 2024

braydonk commented Nov 14, 2024


		`tasks.mak` is provided without any guarantees or warranty on its targets. It is meant purely for developer convenience, and it is advised not to make any dependency on the targets since they are subject to change at any time.

		FLB=$(PWD)/dist/opt/google-cloud-ops-agent/subagents/fluent-bit/bin/fluent-bit \
		OTELOPSCOL=$(PWD)/dist/opt/google-cloud-ops-agent/subagents/opentelemetry-collector/otelopscol \

Add transformation test make target and docs #1722

Are you sure you want to change the base?

Add transformation test make target and docs #1722

Conversation

braydonk commented May 23, 2024

Description

Related issue

How has this been tested?

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

braydonk May 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

braydonk commented Nov 14, 2024

braydonk May 24, 2024 •

edited

Loading