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

Linode lke #818 #904

Merged
merged 26 commits into from
Sep 15, 2023
Merged

Linode lke #818 #904

merged 26 commits into from
Sep 15, 2023

Conversation

professorabhay
Copy link
Contributor

@professorabhay professorabhay commented Jul 24, 2023

Problem

#818

Checklist

  • Code follows the contributing guidelines
  • Changes have been thoroughly tested
  • Documentation has been updated, if necessary
  • Any dependencies have been added to the project, if necessary

Reviewers

@ShubhamPalriwala and @mlabouardy

Copy link
Contributor

@ShubhamPalriwala ShubhamPalriwala left a comment

Choose a reason for hiding this comment

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

LGTM thanks to @mlabouardy! 🚀
hey @professorabhay just suggested a couple of small nits (apologies for asking such small changes), once done, we can proceed and merge the PR!

providers/linode/lkepool/lke_pool.go Outdated Show resolved Hide resolved
providers/linode/lkepool/lke_pool.go Outdated Show resolved Hide resolved
Cost: 0,
// Name: nodePool.Label,
FetchedAt: time.Now(),
CreatedAt: time.Time{}, // Update this with the actual created time.
Copy link
Contributor

Choose a reason for hiding this comment

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

can we fix this please if possible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried but not find the way to do so. Because every time I try to fetch then its give error of not defined in linodego library

Copy link
Contributor

Choose a reason for hiding this comment

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

Then can we have some other identifier if possible as a name? or if you could fetch the name in some other way and parse it? because ideally we should not not have a name. Let me know if you need my hand on this!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll let you know

@ShubhamPalriwala ShubhamPalriwala added this to the v3.1.1 milestone Jul 26, 2023
@ShubhamPalriwala
Copy link
Contributor

Hey @professorabhay, any update on this please? If you are unable to get the node name, can you add any other identifier as a name? Because we cannot keep that field blank, we can possibly tackle the name at a later stage! Let me know once the PR is ready for review!

Added identifier "Type" as a name
@professorabhay
Copy link
Contributor Author

Hey @ShubhamPalriwala, I make the changes as you said.
As, I read the official code of the library name is not return in the object.
image
LINK

@ShubhamPalriwala
Copy link
Contributor

Okay, so can you add something like an ID to the name field of the Komiser object for a comparatively better identification? Thanks

@professorabhay
Copy link
Contributor Author

Screenshot_2023-08-26-19-35-10-971_com.github.android.png

I added type because the id is fetched in the resource id as per above img. I think that if both fields will same then it create confusion in understanding. But if you still want the update then I can give it a try 🙂

@AvineshTripathi
Copy link
Collaborator

We donot have a standard for now so I would suggest keeping id is better, but @ShubhamPalriwala I think we should work together and write down what exactly this field in the resources should carry

@mlabouardy
Copy link
Collaborator

We donot have a standard for now so I would suggest keeping id is better, but @ShubhamPalriwala I think we should work together and write down what exactly this field in the resources should carry

Great suggestion, happy to help with that as well :)

@mlabouardy mlabouardy merged commit 01a07d9 into tailwarden:develop Sep 15, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants