Skip to content
This repository was archived by the owner on Aug 4, 2020. It is now read-only.

Comments

MGMT-882 Add base URL including protocol to config map#508

Draft
empovit wants to merge 1 commit intofilanov:masterfrom
empovit:vemporop/MGMT-882
Draft

MGMT-882 Add base URL including protocol to config map#508
empovit wants to merge 1 commit intofilanov:masterfrom
empovit:vemporop/MGMT-882

Conversation

@empovit
Copy link
Collaborator

@empovit empovit commented Jul 29, 2020

No description provided.

@empovit empovit requested review from filanov and ronniel1 July 29, 2020 10:34
@ronniel1
Copy link
Collaborator

Where are these environment variables actually used?
Also do we need to keep the host and port environment variables?

@empovit
Copy link
Collaborator Author

empovit commented Jul 29, 2020

@ronniel1 other PRs are coming. I want to avoid a chicken-and-egg problem, so making changes incrementally. My reasoning is make the variable available, change A to use it. B can still use the old ones. Remove when everybody uses the new variable. This is probably because I can't estimate the impact yet.

@filanov filanov requested a review from ori-amizur July 30, 2020 06:51
type InstructionConfig struct {
InventoryURL string `envconfig:"INVENTORY_URL" default:"10.35.59.36"`
InventoryPort string `envconfig:"INVENTORY_PORT" default:"30485"`
InventoryBaseUrl string `envconfig:"INVENTORY_BASE_URL" default:"http://10.35.59.36:30485"`
Copy link
Owner

Choose a reason for hiding this comment

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

Default is not needed - it's incorrect anyway.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was trying to keep the existing functionality. Do you want me to remove it?

Copy link
Owner

Choose a reason for hiding this comment

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

i was talking about the default values, once the agent and test infra ready to this change you can remove those as well.

ReleaseImage string `envconfig:"OPENSHIFT_INSTALL_RELEASE_IMAGE" default:"quay.io/openshift-release-dev/ocp-release@sha256:eab93b4591699a5a4ff50ad3517892653f04fb840127895bb3609b3cc68f98f3"`
InventoryURL string `envconfig:"INVENTORY_URL" default:"10.35.59.36"`
InventoryPort string `envconfig:"INVENTORY_PORT" default:"30485"`
InventoryBaseURL string `envconfig:"INVENTORY_BASE_URL" default:"http://10.35.59.36:30485"`
Copy link
Owner

Choose a reason for hiding this comment

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

Default is not needed - it's incorrect anyway.

@empovit empovit requested review from filanov and ronniel1 July 30, 2020 19:00
@empovit empovit marked this pull request as draft July 30, 2020 20:24
Copy link
Collaborator

@YuviGold YuviGold left a comment

Choose a reason for hiding this comment

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

Go over your changed files - All the protocol references should be either http or https

@empovit
Copy link
Collaborator Author

empovit commented Aug 2, 2020

@YuviGold I'm not sure I understand your comment. The host and port parameters have been consolidated into a single URL parameter, with the default value of http://10.35.59.36:30485

Copy link
Collaborator

@YuviGold YuviGold left a comment

Choose a reason for hiding this comment

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

LGTM

@tsorya
Copy link
Collaborator

tsorya commented Aug 2, 2020

@empovit it will need test-infra-change too

@empovit
Copy link
Collaborator Author

empovit commented Aug 2, 2020

@tsorya yes, thanks for reminding me! The commit is ready, I'm waiting with a PR :) empovit/assisted-test-infra@3c3cccc

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants