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

Tiobe Integration #420

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

kadinsayani
Copy link

@kadinsayani kadinsayani commented Oct 9, 2024

This PR adds code coverage for unit tests and snap system tests.

@kadinsayani kadinsayani force-pushed the tiobe-integration branch 2 times, most recently from 3c9ee1a to b78a357 Compare October 9, 2024 20:51
tomponline
tomponline previously approved these changes Oct 10, 2024
Copy link
Member

@tomponline tomponline left a comment

Choose a reason for hiding this comment

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

Let me know when you need secrets.TICS_AUTH_TOKEN setup

@kadinsayani kadinsayani changed the title Unit Test Tiobe Integration Tiobe Integration Oct 10, 2024
@kadinsayani
Copy link
Author

kadinsayani commented Oct 10, 2024

For the system tests using the MicroCloud snap, it looks like the only requirements to generate the coverage files are:

  • GOCOVERDIR needs to be set in the snap confinement space - I'm creating a directory on the runner at /var/snap/microcloud/common/data/coverage; and
  • The side loaded binary should be compiled with the -cover flag.

@tomponline, once I've got it working here, we can utilize a similar approach for LXD.

@kadinsayani kadinsayani force-pushed the tiobe-integration branch 2 times, most recently from 5f8676c to 9a0ecdf Compare October 14, 2024 18:43
@kadinsayani kadinsayani marked this pull request as draft October 14, 2024 18:44
tomponline added a commit to canonical/microcloud-pkg-snap that referenced this pull request Oct 16, 2024
This PR creates a GOCOVERDIR in the snap confinement space when using
debug binaries.

Note: required for canonical/microcloud#420.
@kadinsayani kadinsayani marked this pull request as ready for review October 16, 2024 19:28
@kadinsayani
Copy link
Author

kadinsayani commented Oct 16, 2024

There appears to be a problem with the MicroCloud CI tests because of a regression in upstream Ceph versioning. As a result, I haven’t tested this yet.

@kadinsayani kadinsayani force-pushed the tiobe-integration branch 2 times, most recently from 608198c to 519061b Compare October 18, 2024 17:31
test/main.sh Fixed Show fixed Hide fixed
@kadinsayani kadinsayani force-pushed the tiobe-integration branch 2 times, most recently from ced5879 to d038f14 Compare October 18, 2024 19:22
@kadinsayani
Copy link
Author

kadinsayani commented Oct 18, 2024

@kadinsayani
Copy link
Author

Good to go on this, here's an example successful run on my fork: https://github.com/kadinsayani/microcloud/actions/runs/11413516898/job/31763088034.

tomponline
tomponline previously approved these changes Oct 19, 2024
@kadinsayani
Copy link
Author

kadinsayani commented Oct 19, 2024

We recently started using the go1.23.1 toolchain for MicroCloud. To use StaticCheck with Go 1.23, it needs to be built with Go 1.23. Hence why the Tics job throws errors like this one:

2024-10-19 04:00:23.592: [INFO 3012] Analyzing 30/58 /home/runner/work/microcloud/microcloud/cmd/microcloud/session.go
start analysis: 2024-10-19 04:00:23.592
  Initialization
  Initialization time: 0.001 sec
Error: [ERROR 5073] The following error(s) occurred while 'StaticCheck' was running: '/home/runner/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.23.1.linux-amd64/src/maps/iter.go:51:20: cannot range over seq (variable of type iter.Seq2[K, V])'.

These errors are what caused the downtick in TQI scores across various Tiobe quality metrics. The analyzer runs are erroring out, so data isn't being sent to Tics for the coding standards runs.

For the security checks, I'm seeing the following error on every run:

[ERROR 5255] Command 'cov-build' failed with exit status 256, error: -fno-stack-protector ./cgo.go See log '/tmp/tics/11413516898_1_tics_tics-github-action/ticstmpdir/Coverity/cov-build-console.log'.

Tics uses Coverity for security scanning, here's something I found from their docs:

By default, cov-build returns the exit code from the native build.

Exit code 256 typically indicates that the system can't locate the binary to run the command, so I think this is also a dependency issue.

I propose that we bump the Go version in the Tics job to ensure we capture the right dependencies for Tics at the Install dependencies step. Testing now: https://github.com/kadinsayani/microcloud/actions/runs/11420168463.

@tomponline
Copy link
Member

Yeah we should switch to go 1.23

@kadinsayani
Copy link
Author

kadinsayani commented Oct 21, 2024

Good to go - all of the Tiobe runs are operational and the quality metrics are accurate.

https://github.com/kadinsayani/microcloud/actions/runs/11432414984/job/31830683250

Copy link
Contributor

@roosterfish roosterfish left a comment

Choose a reason for hiding this comment

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

Thanks for this! A few comments from my side, mostly for clarification.

@@ -271,8 +271,8 @@ set_debug_binaries() {
lxc exec "${name}" -- rm -f /var/snap/microcloud/common/microcloudd.debug
lxc exec "${name}" -- rm -f /var/snap/microcloud/common/microcloud.debug

lxc file push --quiet "${MICROCLOUDD_DEBUG_PATH}" "${name}"/var/snap/microcloud/common/microcloudd.debug
lxc file push --quiet "${MICROCLOUD_DEBUG_PATH}" "${name}"/var/snap/microcloud/common/microcloud.debug
lxc file push --quiet "${MICROCLOUDD_DEBUG_PATH}" "${name}"/var/snap/microcloud/common/microcloudd.debug --mode 0755
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of interest what is the reason for changing the permission for the debug binaries?

Copy link
Author

Choose a reason for hiding this comment

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

lxc file push sets the file mode to 0644 by default - we want to make sure the binaries are executable and maintain the file mode set by the Go compiler. We also check for the presence of these executables in commands/daemon.start and commands/microcloud in the MicroCloud snap. When they are present and executable, the GOCOVERDIR is created. Here's the relevant PR: canonical/microcloud-pkg-snap#15.

@@ -0,0 +1,3 @@
# Checks being ignored:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this required?

Copy link
Author

Choose a reason for hiding this comment

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

We wanted to see if adding a staticcheck.conf with this rule would silence the Coding standards violations, but it looks like Tics isn't picking this up so our Coding standards TQI score remains the same. See https://github.com/kadinsayani/microcloud/actions/runs/11446739960/job/31905615699.

Copy link
Author

@kadinsayani kadinsayani Oct 22, 2024

Choose a reason for hiding this comment

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

We can still keep this staticcheck rule since the violation doesn't apply to us.

@@ -192,6 +192,8 @@ run_test() {
${TEST_CURRENT}
END_TIME="$(date +%s)"

collect_go_cover_files
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it not sufficient to do the collection here but also in reset_systems and restore_systems?

Copy link
Author

Choose a reason for hiding this comment

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

A few of the test suites use reset_systems during test cases. reset_systems may call restore_systems when SNAPSHOT_RESTORE is set to 1.

.github/workflows/tests.yml Show resolved Hide resolved
@kadinsayani kadinsayani force-pushed the tiobe-integration branch 5 times, most recently from 7b37e0f to 61a0ec4 Compare October 23, 2024 14:33
Signed-off-by: Kadin Sayani <kadin.sayani@canonical.com>
@kadinsayani
Copy link
Author

I've rebased and resolved a naming issue with the coverage file directories. Test run: https://github.com/kadinsayani/microcloud/actions/runs/11493847667/job/31990262151.

@kadinsayani
Copy link
Author

I've rebased and resolved a naming issue with the coverage file directories. Test run: https://github.com/kadinsayani/microcloud/actions/runs/11493847667/job/31990262151.

Looks like there's still an issue with the naming. I'll try and fix this some time today. Just need a unique identifier to append so there aren't naming conflicts.

Signed-off-by: Kadin Sayani <kadin.sayani@canonical.com>
Signed-off-by: Kadin Sayani <kadin.sayani@canonical.com>
@kadinsayani
Copy link
Author

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.

3 participants