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

fixes : #1177 support variable values on open libery alizer's port de… #111

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

Conversation

Horiodino
Copy link

Description of Changes

resolves the port value by checking if it is a variable and if it is, it returns the value of the variable

Related Issue(s)

devfile/api#1177

Acceptance Criteria

Testing and documentation do not need to be complete in order for this PR to be approved. However, tracking issues must be opened for missing testing/documentation.

  • Unit/Functional tests

  • Documentation

Tests Performed

✔️️ Added suitable test case based on Issue.

How To Test

Instructions for the reviewer on how to test your changes.

Notes To Reviewer

Any notes you would like to include for the reviewer.

Copy link
Contributor

@thepetk thepetk left a comment

Choose a reason for hiding this comment

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

I've left a minor comment. Overall looks good to me, I only think we should keep the existing flow of testing and simply add more test cases in the component_recognizer_test.go

}
defer os.RemoveAll(tempDir)

hardcodedXML := `
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be better here for consistency reasons to keep the already existing flow by adding a test in https://github.com/devfile/alizer/blob/main/test/apis/component_recognizer_test.go#L159-L161 instead of creating the openliberty_detector_test.go

Copy link
Contributor

@Jdubrick Jdubrick left a comment

Choose a reason for hiding this comment

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

Overall I like the changes :) just left 1 comment regarding adding a few more test cases to cover all scenarios


ctx := context.TODO()

t.Run("Hardcoded Ports", func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to add a test case for a mix of hardcoded and variable based ports?

Copy link
Contributor

Choose a reason for hiding this comment

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

Possibly one for empty var as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 on the suggested test cases, as mentioned in #111 (comment) I'd prefer following the https://github.com/devfile/alizer/blob/main/test/apis/component_recognizer_test.go#L159-L161 example as the format of the tests.

@Jdubrick
Copy link
Contributor

/ok-to-test

Copy link

openshift-ci bot commented Oct 18, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Horiodino
Once this PR has been reviewed and has the lgtm label, please ask for approval from jdubrick. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants