Add Custom Link and Webhook initializers #391

MajesticFalcon · 2021-01-16T05:17:20Z

Related Issue:

New Behavior

Custom link and web hooks can now be created via startup scripts.
...

Contrast to Current Behavior

Current implementation is missing this behavior.
...

Discussion: Benefits and Drawbacks

...

Changes to the Wiki

...

Proposed Release Note Entry

Added custom_link initializer
Added webhook initializer
...

Double Check

I have read the comments and followed the PR template.
I have explained my PR according to the information in the comments.
My PR targets the develop branch.

MajesticFalcon · 2021-01-16T05:21:07Z

@cimnine, as requested, I pulled the new features into a new branch. The code was already outdated so I also updated the startup scripts and added error handling, icon printing, etc.

fix comment

…latest netbox release

…ents

initializers/custom_links.yml

startup_scripts/280_custom_links.py

startup_scripts/290_webhooks.py

initializers/custom_links.yml

cimnine

Thank you for the contribution! This could one day even the way to do full fledged integration testing of the webhooks feature. Really cool!

Please also see my previous feedback. I would like to merge this in the version we're currently preparing. If you can't look at the feedback right now (that's perfectly ok!), I'd appreciate a quick message so that we can get all the other changes out.

initializers/custom_links.yml

initializers/webhooks.yml

startup_scripts/290_webhooks.py

Co-authored-by: Christian Mäder <cimnine@users.noreply.github.com>

MajesticFalcon · 2021-01-21T02:32:07Z

Thank you for the contribution! This could one day even the way to do full fledged integration testing of the webhooks feature. Really cool!

Please also see my previous feedback. I would like to merge this in the version we're currently preparing. If you can't look at the feedback right now (that's perfectly ok!), I'd appreciate a quick message so that we can get all the other changes out.

I typically get on around 8pm CST. I am just seeing this, I apologize if this is late notice. I can fix it tonight.

Co-authored-by: Christian Mäder <cimnine@users.noreply.github.com>

Move to a more standard method of object handling

cimnine · 2021-01-21T07:45:10Z

I typically get on around 8pm CST. I am just seeing this, I apologize if this is late notice. I can fix it tonight.

No need to apologize. I don't expect people to have time immediately. I just wanted to know whether we should proceed with the release or if you think you will have time soon-ish to look at the comments :)

MajesticFalcon · 2021-01-21T14:37:46Z

I typically get on around 8pm CST. I am just seeing this, I apologize if this is late notice. I can fix it tonight.

No need to apologize. I don't expect people to have time immediately. I just wanted to know whether we should proceed with the release or if you think you will have time soon-ish to look at the comments :)

Thank you for your help! Is there anything left to do before merging this in?

cimnine

Thank you for your effort to improve and fix the code. I've again added some recommendations. If you find them actionable, please make sure to check the rest of this PR's changes if these remarks apply there as well, as I typically don't add a remark twice when it's the identical remark, just on a different subset of the code.
Otherwise I welcome your argument wherever you disagree :)

cimnine · 2021-01-22T09:16:58Z

initializers/custom_links.yml

+#  - name: link_to_localhost
+#  text: 'Link to the users localhost'
+#  url: 'http://localhost'
+#  new_window: True
+#  content_type: device


This part is still wrong:

Suggested change

# - name: link_to_localhost

# text: 'Link to the users localhost'

# url: 'http://localhost'

# new_window: True

# content_type: device

# - name: link_to_localhost

# text: 'Link to the users localhost'

# url: 'http://localhost'

# new_window: True

# content_type: device

You can run the test locally to see if your changes work and integrate with all the other initializers:

./build-latest.sh && ./test.sh latest

cimnine · 2021-01-22T09:18:53Z

startup_scripts/280_custom_links.py

+  if link['content_type_id'] is not None:
+    custom_link = CustomLink(**link)
+    if not CustomLink.objects.filter(name=custom_link.name):
+      custom_link.save()


You did change the implementation to get_or_create after my comment in 290_webhooks.py – didn't that work with custom links, was this just an oversight or didn't you intentionally not change it here?

cimnine · 2021-01-22T09:22:54Z

startup_scripts/280_custom_links.py

+for link in custom_links:
+  content_type = link.pop('content_type')
+  link['content_type_id'] = get_content_type_id(content_type)
+  if link['content_type_id'] is not None:


I suggest to change the logic here as follows:

Suggested change

if link['content_type_id'] is not None:

if link['content_type_id'] is None:

print("⚠️ Unable to create Custom Link '{0}': The content_type '{1}' is unknown".format(link.name, content_type))

continue

This will make it obvious which custom link failed to get created and why.

cimnine · 2021-01-22T09:26:43Z

startup_scripts/280_custom_links.py

+  try:
+    return ContentType.objects.get(model=content_type_str).id
+  except ContentType.DoesNotExist:
+    print("⚠️ Error determining content type id for user declared var: {0}".format(content_type_str))


Suggested change

print("⚠️ Error determining content type id for user declared var: {0}".format(content_type_str))

return None

... or ...

Suggested change

print("⚠️ Error determining content type id for user declared var: {0}".format(content_type_str))

pass

... (not sure which is more idiomatic in Python).

The error is technically accurate, but not actionable for our users.
They will not know what went wrong. And because we don't have access to the context here (i.e. the custom_link.name is not known, and it would be a drag to pass it to this function just in case there's an error), I would skip a message here and replace it with an error in the caller. (As per above suggestion.)

cimnine · 2021-01-22T09:29:23Z

startup_scripts/280_custom_links.py

+    custom_link = CustomLink(**link)
+    if not CustomLink.objects.filter(name=custom_link.name):
+      custom_link.save()
+      print("🖥️  Created Custom Link {0}".format(custom_link.name))


Suggested change

print("🖥️ Created Custom Link {0}".format(custom_link.name))

print("🖥️ Created Custom Link '{0}'".format(custom_link.name))

It's cool to have this additional information, which custom link was created. Though it's good habit to enclose strings into quotation signs when printing them to stdout. Otherwise it's really hard to see if there is e.g. an extra whitespace where there shouldn't have been one.

cimnine · 2021-02-08T09:37:53Z

Merged with commit f33c647

MajesticFalcon changed the title ~~Feature~~ Feature - Custom Link and Webhook initializers Jan 16, 2021

MajesticFalcon added 6 commits January 15, 2021 23:40

add custom link initializers

f7879f0

add webhook initializer

a1c7f72

Update webhooks.yml

24786c6

fix comment

fix webhook initializer yaml and webhook startup script to work with …

717575f

…latest netbox release

add error handling for webhook and custom links. fix initializer comm…

3250001

…ents

add icons to help messages

1ad1faf

MajesticFalcon force-pushed the feature branch from d6e11ff to 1ad1faf Compare January 16, 2021 05:41

replace loop with list comprehension

9d2d5f0

cimnine added the enhancement The issue describes an enhancement that we would like to implement in the future. label Jan 19, 2021

cimnine added this to the 0.28.0 milestone Jan 19, 2021

cimnine changed the base branch from release to develop January 20, 2021 08:07

cimnine requested changes Jan 20, 2021

View reviewed changes

cimnine changed the title ~~Feature - Custom Link and Webhook initializers~~ Add Custom Link and Webhook initializers Jan 20, 2021

cimnine mentioned this pull request Jan 20, 2021

Prepare version 1.0.0 #394

Merged

3 tasks

cimnine reviewed Jan 20, 2021

View reviewed changes

initializers/custom_links.yml Outdated Show resolved Hide resolved

cimnine requested changes Jan 20, 2021

View reviewed changes

initializers/custom_links.yml Show resolved Hide resolved

initializers/webhooks.yml Outdated Show resolved Hide resolved

startup_scripts/290_webhooks.py Outdated Show resolved Hide resolved

startup_scripts/290_webhooks.py Outdated Show resolved Hide resolved

cimnine added a commit that referenced this pull request Jan 20, 2021

Fixes and Suggestions for PR #391

3f67936

MajesticFalcon and others added 5 commits January 20, 2021 15:47

Update startup_scripts/280_custom_links.py

b2b974a

Co-authored-by: Christian Mäder <cimnine@users.noreply.github.com>

Update startup_scripts/280_custom_links.py

aca5910

Co-authored-by: Christian Mäder <cimnine@users.noreply.github.com>

Update startup_scripts/290_webhooks.py

f1c9d40

Co-authored-by: Christian Mäder <cimnine@users.noreply.github.com>

Update initializers/custom_links.yml

0ae6653

Co-authored-by: Christian Mäder <cimnine@users.noreply.github.com>

Update initializers/webhooks.yml

042857e

Co-authored-by: Christian Mäder <cimnine@users.noreply.github.com>

MajesticFalcon and others added 2 commits January 20, 2021 21:25

Update initializers/custom_links.yml

efac961

Co-authored-by: Christian Mäder <cimnine@users.noreply.github.com>

Update 290_webhooks.py

1b234cf

Move to a more standard method of object handling

cimnine requested changes Jan 22, 2021

View reviewed changes

cimnine added the awaiting answer There is still some open discussion. label Jan 26, 2021

cimnine modified the milestones: 0.28.0, 1.0.0 Jan 27, 2021

cimnine closed this Feb 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Custom Link and Webhook initializers #391

Add Custom Link and Webhook initializers #391

MajesticFalcon commented Jan 16, 2021 •

edited by cimnine

Loading

MajesticFalcon commented Jan 16, 2021

cimnine left a comment

MajesticFalcon commented Jan 21, 2021

cimnine commented Jan 21, 2021 •

edited

Loading

MajesticFalcon commented Jan 21, 2021 •

edited

Loading

cimnine left a comment •

edited

Loading

cimnine Jan 22, 2021

cimnine Jan 22, 2021

cimnine Jan 22, 2021

cimnine Jan 22, 2021

cimnine Jan 22, 2021

cimnine commented Feb 8, 2021

-  if link['content_type_id'] is not None:
+  if link['content_type_id'] is None:
+    print("⚠️ Unable to create Custom Link '{0}': The content_type '{1}' is unknown".format(link.name, content_type))
+    continue

	print("⚠️ Error determining content type id for user declared var: {0}".format(content_type_str))
	return None

	print("⚠️ Error determining content type id for user declared var: {0}".format(content_type_str))
	pass

	print("🖥️ Created Custom Link {0}".format(custom_link.name))
	print("🖥️ Created Custom Link '{0}'".format(custom_link.name))

Add Custom Link and Webhook initializers #391

Add Custom Link and Webhook initializers #391

Conversation

MajesticFalcon commented Jan 16, 2021 • edited by cimnine Loading

New Behavior

Contrast to Current Behavior

Discussion: Benefits and Drawbacks

Changes to the Wiki

Proposed Release Note Entry

Double Check

MajesticFalcon commented Jan 16, 2021

cimnine left a comment

Choose a reason for hiding this comment

MajesticFalcon commented Jan 21, 2021

cimnine commented Jan 21, 2021 • edited Loading

MajesticFalcon commented Jan 21, 2021 • edited Loading

cimnine left a comment • edited Loading

Choose a reason for hiding this comment

cimnine Jan 22, 2021

Choose a reason for hiding this comment

cimnine Jan 22, 2021

Choose a reason for hiding this comment

cimnine Jan 22, 2021

Choose a reason for hiding this comment

cimnine Jan 22, 2021

Choose a reason for hiding this comment

cimnine Jan 22, 2021

Choose a reason for hiding this comment

cimnine commented Feb 8, 2021

MajesticFalcon commented Jan 16, 2021 •

edited by cimnine

Loading

cimnine commented Jan 21, 2021 •

edited

Loading

MajesticFalcon commented Jan 21, 2021 •

edited

Loading

cimnine left a comment •

edited

Loading