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.

Sign up for GitHub

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

Jump to bottom

Centralize environment configuration #6293

Merged
merged 21 commits into from
Nov 17, 2022
Merged

Centralize environment configuration #6293

merged 21 commits into from
Nov 17, 2022

Conversation

feloy
Copy link
Contributor

@feloy feloy commented Nov 8, 2022 •
edited
Loading

What type of PR is this:

/kind feature

What does this PR do / why we need it:

Which issue(s) this PR fixes:

Fixes #6149

PR acceptance criteria:

  • Unit test

  • Integration test

  • Documentation

How to test changes / Special notes to the reviewer:

See this wiki page for more information: https://github.com/redhat-developer/odo/wiki/Dev:-Code-architecture

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation labels Nov 8, 2022
@openshift-ci openshift-ci bot requested review from dharmit and rm3l November 8, 2022 08:45
@netlify
Copy link

netlify bot commented Nov 8, 2022 •
edited
Loading

✅ Deploy Preview for odo-docusaurus-preview ready!

Name Link
🔨 Latest commit e0a7f88
🔍 Latest deploy log https://app.netlify.com/sites/odo-docusaurus-preview/deploys/637606cd7ea09b0009e5ba4d
😎 Deploy Preview https://deploy-preview-6293--odo-docusaurus-preview.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 settings.

@odo-robot
Copy link

odo-robot bot commented Nov 8, 2022 •
edited
Loading

NoCluster Tests on commit 6cb890c finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Nov 8, 2022 •
edited
Loading

Unit Tests on commit 6cb890c finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Nov 8, 2022 •
edited
Loading

Validate Tests on commit 6cb890c finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Nov 8, 2022 •
edited
Loading

Windows Tests (OCP) on commit 6cb890c finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Nov 8, 2022 •
edited
Loading

Kubernetes Tests on commit 6cb890c finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Nov 8, 2022 •
edited
Loading

OpenShift Tests on commit 6cb890c finished successfully.
View logs: TXT HTML

@feloy feloy force-pushed the feat-6149/central-config branch 2 times, most recently from 2544714 to 6436cde Compare November 8, 2022 12:55
@feloy feloy closed this Nov 9, 2022
@feloy feloy reopened this Nov 9, 2022
@feloy feloy changed the title [WIP] Centralize environment configuration Centralize environment configuration Nov 9, 2022
@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. Required by Prow. label Nov 9, 2022
@feloy
Copy link
Contributor Author

feloy commented Nov 9, 2022

Rebased on main to use go v1.18

@feloy feloy closed this Nov 14, 2022
@feloy feloy reopened this Nov 14, 2022
@valaparthvi valaparthvi self-assigned this Nov 14, 2022
rm3l
rm3l reviewed Nov 14, 2022
View reviewed changes
go.mod Outdated Show resolved Hide resolved
pkg/devfile/image/image.go Outdated Show resolved Hide resolved
valaparthvi
valaparthvi suggested changes Nov 14, 2022
View reviewed changes
tests/helper/helper_telemetry.go
Expect(err).To(BeNil())
tempFile, err := ioutil.TempFile("", "telemetry")
Expect(err).NotTo(HaveOccurred())
Expect(setDebugTelemetryFile(tempFile.Name())).NotTo(HaveOccurred())
Expect(tempFile.Close()).NotTo(HaveOccurred())
}

func GetDebugTelemetryFile() string {
return os.Getenv(DebugTelemetryFileEnv)
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this env var be obtained via envcontext or are you using the library since it's a test helper?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In integration tests, we don't need to rely on the envcontext package

tests/helper/helper_telemetry.go
func setDebugTelemetryFile(value string) error {
return os.Setenv(segment.DebugTelemetryFileEnv, value)
return os.Setenv(DebugTelemetryFileEnv, value)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you not using envcontext.WithConfig to set this env var?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are integration tests (we are not inside odo), we need to check that odo reads the values from the environment variables

@feloy feloy closed this Nov 14, 2022
@feloy feloy reopened this Nov 14, 2022
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Nov 17, 2022
@feloy
Copy link
Contributor Author

feloy commented Nov 17, 2022

Rebased to fix conflict on pkg/dev/dev.go

rm3l
rm3l approved these changes Nov 17, 2022
View reviewed changes
Copy link
Member

@rm3l rm3l left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM - thanks.

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Nov 17, 2022
rm3l
rm3l reviewed Nov 17, 2022
View reviewed changes
Copy link
Member

@rm3l rm3l left a comment

Choose a reason for hiding this comment

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

LGTM feature-wise.

/approve

@openshift-ci
Copy link

openshift-ci bot commented Nov 17, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rm3l

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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Nov 17, 2022
@feloy feloy closed this Nov 17, 2022
@rm3l
Copy link
Member

rm3l commented Nov 17, 2022

Kubernetes Tests failed rather quickly with the following error on cloning the repo:

Preparing to start the job...
Running on public worker: eu-depublicworker05-3
Pulling image quay.io/odo-dev/ibmcloudtest-image:v1.18-1 ...
Job execution started at Thu Nov 17 10:14:38 UTC 2022
Custom image
Cloning the 'main' branch from repo 'https://github.com/redhat-developer/odo.git'
fatal: unable to access 'https://github.com/redhat-developer/odo.git/': Failed to connect to github.com port 443: Connection timed out
Clone was not successful. Code 128 - Retrying shortly...
fatal: unable to access 'https://github.com/redhat-developer/odo.git/': gnutls_handshake() failed: Error in the pull function.
2022/11/17 10:18:58 Skipping step because a previous step failed
2022/11/17 10:18:59 Skipping step because a previous step failed

Finished: FAILED

Let's trigger them again...

@feloy feloy reopened this Nov 17, 2022
@feloy feloy closed this Nov 17, 2022
@feloy feloy reopened this Nov 17, 2022
@feloy
Copy link
Contributor Author

feloy commented Nov 17, 2022

Removed old namepaces on Windows cluster before restarting the tests

@feloy feloy closed this Nov 17, 2022
@feloy feloy reopened this Nov 17, 2022
@sonarcloud
Copy link

sonarcloud bot commented Nov 17, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@feloy
Copy link
Contributor Author

feloy commented Nov 17, 2022

/override ci/prow/v4.11-integration-e2e
Tests pass on IBM Cloud

@openshift-ci
Copy link

openshift-ci bot commented Nov 17, 2022

@feloy: Overrode contexts on behalf of feloy: ci/prow/v4.11-integration-e2e

In response to this:

/override ci/prow/v4.11-integration-e2e
Tests pass on IBM Cloud

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-merge-robot openshift-merge-robot merged commit 712254a into redhat-developer:main Nov 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rm3l rm3l rm3l approved these changes

@dharmit dharmit Awaiting requested review from dharmit

@valaparthvi valaparthvi Awaiting requested review from valaparthvi

Assignees

@rm3l rm3l

@valaparthvi valaparthvi

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation lgtm Indicates that a PR is ready to be merged. Required by Prow.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Centralize environment variable based configuration
4 participants
@feloy @rm3l @valaparthvi @openshift-merge-robot