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

feat: Make startup scripts idempotent #729

Merged

Conversation

kr3ator
Copy link
Contributor

@kr3ator kr3ator commented Apr 5, 2022

Related Issue:

New Behavior

This PR makes sure that startup scripts won't crash on subsequent runs if some of the attributes of a given object have been changed in the database since first run of the scripts.

New function has been introduced to separate the matching_params which uniquely identify given object from other non-relevant fields provided in the params dictionary.

Contrast to Current Behavior

Currently the get_or_create method tries to get the object matching all fields provided in YAML initializer.
Problem arises when after initial object creation some of the fields are changed in the database, e.g.

Fields that make a unique match on an object are set in the match_params list on a per script basis, with a default value of ["name", "slug"] if not specified.

  1. Startup script creates a 'fabric' tag based on the below YAML initializer:
- name: fabric
  slug: fabric
  color: 3f51b5
  1. Then inside Netbox, we change the color to gray (9e9e9e)
  2. If the container is restarted, the startup_script launches again and tries to do:
Tag.objects.get_or_create(name='fabric', slug='fabric', color='3f51b5')
  1. The get portion of the method fails since the color is different now.
  2. The create portion of the method also fails because tag with name='fabric' & slug='fabric' already exists.

Discussion: Benefits and Drawbacks

Above section sums up the motivation and advantages of this PR. This change is backwards compatible.

Changes to the Wiki

n/a

Proposed Release Note Entry

Feature: Make subsequent startup scripts runs idempotent.

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.

@tobiasge
Copy link
Member

tobiasge commented Apr 5, 2022

Cloud you also run isort and black to ensure a consistent code format.

isort startup_scripts/*py startup_scripts/startup_script_utils/*py
black startup_scripts/*py startup_scripts/startup_script_utils/*py

@kr3ator kr3ator force-pushed the feature/separate_default_params branch from 41f24c6 to d7d7b1e Compare April 5, 2022 14:16
@kr3ator
Copy link
Contributor Author

kr3ator commented Apr 5, 2022

Blacked and sorted.

@kr3ator kr3ator force-pushed the feature/separate_default_params branch 2 times, most recently from 852bc83 to 61f7f6c Compare April 5, 2022 16:09
startup_scripts/startup_script_utils/utils.py Outdated Show resolved Hide resolved
startup_scripts/000_users.py Show resolved Hide resolved
startup_scripts/startup_script_utils/__init__.py Outdated Show resolved Hide resolved
@kr3ator kr3ator force-pushed the feature/separate_default_params branch 2 times, most recently from 72a4206 to a652f47 Compare April 6, 2022 14:41
@kr3ator
Copy link
Contributor Author

kr3ator commented Apr 6, 2022

Hmm mypy linting fails same as in my other PR in which I fixed that in this commit.

If we accept #730 and merge to develop I can then rebase this PR.

@kr3ator kr3ator requested a review from tobiasge April 7, 2022 17:42
@kr3ator kr3ator force-pushed the feature/separate_default_params branch from a652f47 to 9be7b0e Compare April 7, 2022 17:47
@tobiasge
Copy link
Member

tobiasge commented Apr 8, 2022

Looks OK to me.
@cimnine Could also take a look?

@kr3ator kr3ator mentioned this pull request Apr 8, 2022
3 tasks
@kr3ator kr3ator force-pushed the feature/separate_default_params branch from 6a08f45 to 9be7b0e Compare April 8, 2022 12:30
@cimnine cimnine added the enhancement The issue describes an enhancement that we would like to implement in the future. label Apr 8, 2022
Copy link
Collaborator

@cimnine cimnine left a comment

Choose a reason for hiding this comment

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

Based on what criteria did you decide what the match_params should contain?

initializers/users.yml Outdated Show resolved Hide resolved
startup_scripts/startup_script_utils/utils.py Show resolved Hide resolved
@cimnine
Copy link
Collaborator

cimnine commented Apr 8, 2022

Thanks for the PR. That's truely an enhancement and I believe you have found an elegant solution.

Co-authored-by: Christian Mäder <[email protected]>
@kr3ator
Copy link
Contributor Author

kr3ator commented Apr 8, 2022

Based on what criteria did you decide what the match_params should contain?

I've analyzed deeply all the models and it turned out that 90% of the cases the "name" and "slug" parameters are enough to select an object.
For some models it was a bit of trial and error as + analisys of source code of what is a unique set for a given model.

@tobiasge tobiasge merged commit 5c4a1cc into netbox-community:develop Apr 8, 2022
@kr3ator kr3ator deleted the feature/separate_default_params branch April 12, 2022 09:49
@tobiasge tobiasge mentioned this pull request Jul 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement The issue describes an enhancement that we would like to implement in the future.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants