Skip to content

Conversation

camilamacedo86
Copy link
Contributor

@camilamacedo86 camilamacedo86 commented Nov 13, 2024

  • Adds a GitHub Action to validate the installation process in CI, detecting issues in scripts/install.tpl.sh.
  • This workflow follows steps from the Getting Started guide, reducing installation errors.

Motivation:
The error seen in PR #1429 would have been detected with this action in place.

@camilamacedo86 camilamacedo86 requested a review from a team as a code owner November 13, 2024 09:22
Copy link

netlify bot commented Nov 13, 2024

Deploy Preview for olmv1 ready!

Name Link
🔨 Latest commit 1bd3281
🔍 Latest deploy log https://app.netlify.com/sites/olmv1/deploys/6738a7cad8442900087ff255
😎 Deploy Preview https://deploy-preview-1450--olmv1.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@camilamacedo86 camilamacedo86 changed the title 🌱 (ci): Add GitHub Action to validate installation WIP 🌱 (ci): Add GitHub Action to validate installation Nov 13, 2024
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 13, 2024
@camilamacedo86 camilamacedo86 force-pushed the fix-install branch 6 times, most recently from f9bc19e to 43a4410 Compare November 13, 2024 10:35
@camilamacedo86 camilamacedo86 changed the title WIP 🌱 (ci): Add GitHub Action to validate installation 🌱 fix: Makefile docker build command and add GitHub Action to validate installation Nov 13, 2024
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 13, 2024
@camilamacedo86 camilamacedo86 changed the title 🌱 fix: Makefile docker build command and add GitHub Action to validate installation WIP 🌱 fix: Makefile docker build command and add GitHub Action to validate installation Nov 13, 2024
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 13, 2024
@camilamacedo86 camilamacedo86 changed the title WIP 🌱 fix: Makefile docker build command and add GitHub Action to validate installation 🌱 fix: Makefile docker build command and add GitHub Action to validate installation Nov 13, 2024
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 13, 2024
@camilamacedo86 camilamacedo86 changed the title 🌱 fix: Makefile docker build command and add GitHub Action to validate installation WIP: 🌱 fix: Makefile docker build command and add GitHub Action to validate installation Nov 13, 2024
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 13, 2024
@camilamacedo86 camilamacedo86 force-pushed the fix-install branch 2 times, most recently from 8278af7 to d4addce Compare November 13, 2024 11:23
@camilamacedo86 camilamacedo86 changed the title WIP: 🌱 fix: Makefile docker build command and add GitHub Action to validate installation 🌱 fix: Makefile docker build command and add GitHub Action to validate installation Nov 13, 2024
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 13, 2024
@joelanford
Copy link
Member

I don't think this will work on MacOS.

On MacOS, when we build a container, we need to specifically cross-compile to GOOS=linux. That's why the go-build-linux targets exist.

@joelanford
Copy link
Member

I think we _can_probably remove the hardcoded GOARCH setting though.

@camilamacedo86 camilamacedo86 force-pushed the fix-install branch 2 times, most recently from d8506db to 6ef6948 Compare November 13, 2024 15:09
@camilamacedo86

This comment was marked as resolved.

@camilamacedo86 camilamacedo86 changed the title 🌱 fix: Makefile docker build command and add GitHub Action to validate installation WIP : 🌱 fix: Makefile docker build command and add GitHub Action to validate installation Nov 13, 2024
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 13, 2024
@camilamacedo86 camilamacedo86 force-pushed the fix-install branch 5 times, most recently from ae0dd26 to 410ebe3 Compare November 13, 2024 15:55
@joelanford
Copy link
Member

@camilamacedo86 That container won't be runnable in a cluster because it is darwin/arm64, which can't run in a linux-based kubernetes environment.

@@ -0,0 +1,51 @@
name: Install
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that we need a dedicated action to run the install script. All e2e runs in CI will use the install script so I would expect install script issues to be present in e2e runs.

Copy link
Contributor Author

@camilamacedo86 camilamacedo86 Nov 13, 2024

Choose a reason for hiding this comment

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

Unfortunately, that doesn't seem accurate. We’ve been addressing issues with the script for exactly this reason. For example, the error in PR #1429 OR #1411 would have prevented the CI from passing. Could you please check and let me know where this test might be duplicated elsewhere?

Copy link
Contributor

Choose a reason for hiding this comment

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

The Makefile as it is on 'main' right now works fine for me on Linux. The test you linked to above is failing because it appears it was before you changed the docker-build line to target the ./bin directory rather than ./bin/linux.

I don't have access to a mac but I don't really see how the changes in this PR are really different from how it worked before--it seems to just be changing where we build the binaries from ./bin/linux to ./bin? And it's also effectively removing the functionality of the existing 'go-build-local' target and replacing it with the current 'go-build-linux,' just renaming them and removing the former since you're still setting GOOS=linux.

It is interesting that the existing go-build-local on main isn't directly used anywhere--I'm assuming it's there for cases where somebody wanted to compile locally and would just locally edit the docker-build target to use 'build' for one-off testing. Maybe we should just make another docker-build target that uses -local, i.e. docker-build-local, if that's a workflow we see people needing/wanting

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @everettraven. We are (in theory) automatically generating a manifest and rendering an install script as part of the e2e.

If there are gaps in what happens with the installation during the e2e, let's fix it in the e2e instead of introducing a separate test.

Copy link
Contributor

Choose a reason for hiding this comment

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

@camilamacedo86 Part of running our e2e tests includes doing, more or less, what you are doing here.

If you look at the test-e2e make target at

# When running the e2e suite, you can set the ARTIFACT_PATH variable to the absolute path
# of the directory for the operator-controller e2e tests to store the artifacts, which
# may be helpful for debugging purposes after a test run.
#
# for example: ARTIFACT_PATH=/tmp/artifacts make test-e2e
.PHONY: test-e2e
test-e2e: KIND_CLUSTER_NAME := operator-controller-e2e
test-e2e: KUSTOMIZE_BUILD_DIR := config/overlays/e2e
test-e2e: GO_BUILD_FLAGS := -cover
test-e2e: run image-registry e2e e2e-coverage kind-clean #HELP Run e2e test suite on local kind cluster
and dig even further into the run target at
.PHONY: run
run: docker-build kind-cluster kind-load kind-deploy #HELP Build the operator-controller then deploy it into a new kind cluster.
you can see that it runs the make targets docker-build, kind-cluster, kind-load, and kind-deploy. The kind-deploy target at
.PHONY: kind-deploy
kind-deploy: export MANIFEST="./operator-controller.yaml"
kind-deploy: manifests $(KUSTOMIZE) #EXHELP Install controller and dependencies onto the kind cluster.
$(KUSTOMIZE) build $(KUSTOMIZE_BUILD_DIR) > operator-controller.yaml
envsubst '$$CATALOGD_VERSION,$$CERT_MGR_VERSION,$$INSTALL_DEFAULT_CATALOGS,$$MANIFEST' < scripts/install.tpl.sh | bash -s
uses the same installation script that we generate for releases.

Essentially, everything you are doing in this CI action is already done as part of all of our e2e runs in CI.

I'm not quite sure why CI didn't catch the issues you linked, but I also never ended up running into those problems. Is it possible that there may be some difference in tooling versions that resulted in you experiencing those issues?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

HI @everettraven

docker-build kind-cluster kind-load kind-deploy

I see that is calling so that is true.
Thank you for point out.

Is it possible that there may be some difference in tooling versions that resulted in you experiencing those issues?

Not that is not really possible
if you revert #1429 OR #1411 you will see that the error occurs always.

So probably we move forward besides the e2e failures.

@camilamacedo86

This comment was marked as resolved.

@camilamacedo86 camilamacedo86 changed the title WIP : 🌱 fix: Makefile docker build command and add GitHub Action to validate installation 🌱 fix: Makefile docker build command and add GitHub Action to validate installation Nov 13, 2024
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 13, 2024
@camilamacedo86 camilamacedo86 changed the title 🌱 fix: Makefile docker build command and add GitHub Action to validate installation 🌱 (ci) add GitHub Action to validate installation Nov 13, 2024
@camilamacedo86
Copy link
Contributor Author

Closing this one.
@everettraven point out that we are doing those tests; #1450 (comment)
if we revert #1429 OR #1411 you will see that the error occurs always.

So, we probably move forward besides the e2e failures.
that is fine

We are currently facing issues where the installation process is breaking, and we are unable to detect these failures in CI. This action introduces a validation step to ensure that we can at least install the solution, validating the installation script provided in the Getting Started guide.

This is a minimal basic test expected to enhance the developer experience (DX) by reducing user frustration with installation issues.
Copy link

codecov bot commented Nov 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 57.20%. Comparing base (63ef902) to head (1bd3281).

❗ There is a different number of reports uploaded between BASE (63ef902) and HEAD (1bd3281). Click for more details.

HEAD has 2 uploads less than BASE
Flag BASE (63ef902) HEAD (1bd3281)
unit 2 1
e2e 2 1
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1450       +/-   ##
===========================================
- Coverage   74.73%   57.20%   -17.53%     
===========================================
  Files          42       42               
  Lines        3241     3241               
===========================================
- Hits         2422     1854      -568     
- Misses        646     1295      +649     
+ Partials      173       92       -81     
Flag Coverage Δ
e2e 0.00% <ø> (-52.33%) ⬇️
unit 57.20% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

4 participants