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

Device import #4

Merged
merged 2 commits into from
Apr 27, 2018
Merged

Device import #4

merged 2 commits into from
Apr 27, 2018

Conversation

pyang55
Copy link
Contributor

@pyang55 pyang55 commented Apr 24, 2018

added import functionality for devices/device groups
added vendor files using golang/dep

@pyang55 pyang55 self-assigned this Apr 24, 2018
@pyang55 pyang55 requested a review from woz5999 April 24, 2018 23:49
d.Set("collector", restResponse.Data.PreferredCollectorId)
d.Set("disable_alerting", restResponse.Data.DisableAlerting)
d.Set("description", restResponse.Data.Description)
d.Set("hostgroup_id", restResponse.Data.HostGroupIds)
Copy link
Contributor

Choose a reason for hiding this comment

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

this will probably have weird implications for dynamic groups. not sure there's really much to be done about it though.

client := m.(*lmv1.DefaultApi)

// if it is a deviceId, we will add the group directly
if checkID(d.Id()) {
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 normal provider practice accept two different possible types of resource identifiers? it's definitely convenient, just not sure if it's a normal best practice.

Copy link
Contributor Author

@pyang55 pyang55 Apr 25, 2018

Choose a reason for hiding this comment

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

im not too sure. I looked for a bit for provider best practices documentation. I know ID is usually standard but since LM doesn't make that easy for users (unless you run api calls or dig deep into the info tab), I added the second option (for ip/fqdn). Figured itd add more benefit than harm

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. In that case, I'd probably be in favor of ditching the ID and just using the display name. No strong preference either way though.

@pyang55 pyang55 merged commit 8850791 into logicmonitor:master Apr 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants