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

Refs #28135 - Remove URL field from GCE info #466

Merged
merged 1 commit into from
Dec 22, 2019

Conversation

ofedoren
Copy link
Member

This should be also cherry-picked into 0.19-stable for 0.19.5.

@xprazak2
Copy link
Contributor

Before:

url-before

After:

url-after

I haven't tested vmware, ec2, ovirt and rackspace - but openstack and libvirt show url correctly and given they all use the same code, I do not see a reason why the rest of the compute resources would not show the field properly.
I think this is ready unless someone wants to test those remaining CRs.

Copy link
Contributor

@shiramax shiramax left a comment

Choose a reason for hiding this comment

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

@ofedoren the fix looks good to me, however, this change will no reflect in the API, is there a possibility to change the zone to URL in the API?

@ofedoren
Copy link
Member Author

@shiramax, I think we could stop including url attribute in JSON responses from API for GCE info, but it isn't backward compatible, is it?

@shiramax
Copy link
Contributor

@ofedoren OK, I checked with foreman, and it seems like this is the best option. thanks @ofedoren !

@kgaikwad
Copy link
Member

@ofedoren, @shiramax,
any updates on this? can we merge this PR?

@ofedoren
Copy link
Member Author

@kgaikwad, sure, if nobody has concerns, it's ready.

@shiramax shiramax merged commit 3d24c11 into theforeman:master Dec 22, 2019
@kgaikwad
Copy link
Member

kgaikwad commented Jan 9, 2020

@shiramax, This change should be also cherry-picked into 0.19-stable.
Please let me know if any separate cherry-pick PR for this change is required.

@ofedoren
Copy link
Member Author

ofedoren commented Jan 9, 2020

@kgaikwad, I think we need a CP for this into 0.19-stable. Do you want to do it or should I?

@kgaikwad
Copy link
Member

kgaikwad commented Jan 9, 2020

@ofedoren,
Either way will work for me. As I didn't see any CP PR so asked. It would be good if you do it as original PR created by you.

ofedoren added a commit to ofedoren/hammer-cli-foreman that referenced this pull request Jan 9, 2020
ofedoren added a commit that referenced this pull request Jan 10, 2020
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.

5 participants