Skip to content
This repository has been archived by the owner on May 30, 2022. It is now read-only.

Hosts UI revamp #552

Merged
merged 5 commits into from
Dec 13, 2021
Merged

Conversation

dottorblaster
Copy link
Contributor

An image is worth a thousand words:

2021-12-10-110245_1252x1381_scrot

Also, bonus point 馃専 鉃★笍 this PR makes the single host UI page Consul-free.

image

web/hosts.go Show resolved Hide resolved
web/services/hosts.go Outdated Show resolved Hide resolved
Copy link
Member

@nelsonkopliku nelsonkopliku left a comment

Choose a reason for hiding this comment

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

Looks good!


type AzureCloudData struct {
VMName string `json:"vmname"`
ResourceGroup string `json:"resourceGroup"`
Copy link
Member

Choose a reason for hiding this comment

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

Aha! camelCase vs snake_case strikes again :trollface:

Unfortunately we are not being consistent in the way we want json to look like.
And I am part of the problem, too 馃槥, for instance in #546 I have used snake_case

We did it differently throughout the code and I think that at a certain point we should sit, make an agreement on the style, and then commit to refactor all the codebase accordingly, which might not be trivial.
For reference: we have also an open issue for this #385

I don't think it is the right time to tackle such a refactor, though we can find an agreement for the immediate future.

I'd like to keep #546 consistent with this PR and since I don't have strong preferences, I can fix it to make camelCase compliant. Would that be ok?

Then we can have a wider discussion and consider refactoring as well.

Copy link
Member

Choose a reason for hiding this comment

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

@nelsonkopliku @dottorblaster in the entities/models i personally have used snake case everywhere, maybe we can keep this consistent at least for this layer and think about a clean-up later. wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

Ok, whatever it works better. Anyway we'd need to find an agreement on such things. Just not now 馃槃

Copy link
Member

@fabriziosestito fabriziosestito Dec 13, 2021

Choose a reason for hiding this comment

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

i like better the camel case honestly, we can refactor all at once laterz

Copy link
Member

Choose a reason for hiding this comment

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

A bit late but ok for me.

@fabriziosestito fabriziosestito merged commit 2f481f9 into trento-project:main Dec 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants