Skip to content

Conversation

@lakshmi-kannan
Copy link
Contributor

Action registration now uses same validation as actions API. This will fix a lot of issues where registration can let some models pass through but actions list API will break.

@lakshmi-kannan lakshmi-kannan force-pushed the fix_action_registration_validation branch from 7063f3e to a5d1661 Compare December 24, 2014 00:39
@Kami
Copy link
Member

Kami commented Dec 24, 2014

Nice.

I believe there is a same issue with rules - we don't perform the same validation on the way in and out.

Copy link
Member

Choose a reason for hiding this comment

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

I find those ifs slightly confusing.

It seems like we always want to use pack argument which is passed to the method so those ifs are redundant / confusing.

Copy link
Member

Choose a reason for hiding this comment

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

Also, action metadata doesn't need and shouldn't define pack attribute anyway, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. That is why all these ifs are needed. If metadata doesn't contain a pack field. we add it. The case where user specifies a pack_field but doesn't match the folder it's coming from is an interesting case. I currently throw an exception. We can silently ignore the pack field and override it with the pack but that might confuse the user.

On a related note, st2 action create /full/path seems to register actions in default pack. In that case, pack field is required if you want that model to go into right pack.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, thanks, I got it - we need to support a use case of specifying a pack in case user explicitly registers an action using the API.

Maybe it would be good to document this here :)

@Kami
Copy link
Member

Kami commented Dec 29, 2014

Tried to test this branch and everything blew up after I ran register-content script.

It looks like that again, same as the previous PR, this branch changes behavior - we don't update existing actions anymore on register content.

This needs to be fixed so we preserve the old behavior with updating actions on registering.

json.
2014-12-29 17:20:02,481 INFO [-] Action <ResourceReference pack=libcloud,name=enable_cdn_for_container,ref=libcloud.enable_cdn_for_container> found. Will be updated from: ActionDB(description="Enable CDN for container and return the CDN URL", enabled=True, entry_point="enable_cdn_for_container.py", id=54917f740640fd34b644066e, name="enable_cdn_for_container", pack="libcloud", parameters={u'credentials': {u'required': True, u'type': u'string', u'description': u'Name of the credentials set (as defined in the config) to use.'}, u'container_name': {u'required': True, u'type': u'string', u'description': u'Name of the container to enable the CDN for.'}}, runner_type={u'name': u'run-python'}) to: {u'name': u'enable_cdn_for_container', u'parameters': {u'credentials': {u'required': True, u'type': u'string', u'description': u'Name of the credentials set (as defined in the config) to use.'}, u'container_name': {u'required': True, u'type': u'string', u'description': u'Name of the container to enable the CDN for.'}}, u'description': u'Enable CDN for container and return the CDN URL', u'enabled': True, u'entry_point': u'enable_cdn_for_container.py', 'pack': 'libcloud', u'runner_type': u'run-python'}
2014-12-29 17:20:02,496 ERROR [-] Failed to write action to db enable_cdn_for_container.
Traceback (most recent call last):
  File "/data/stanley/st2actions/st2actions/bootstrap/actionsregistrar.py", line 71, in _register_action
    model = Action.add_or_update(model)
  File "/data/stanley/st2common/st2common/persistence/__init__.py", line 73, in add_or_update
    model_object = kls._get_impl().add_or_update(model_object)
  File "/data/stanley/st2common/st2common/models/db/__init__.py", line 144, in add_or_update
    instance.save()
  File "/data/stanley/virtualenv/local/lib/python2.7/site-packages/mongoengine/document.py", line 286, in save
    raise NotUniqueError(message % unicode(err))
NotUniqueError: Tried to save duplicate unique keys (E11000 duplicate key error index: st2.action_d_b.$pack_1_name_1  dup key: { : "libcloud", : "enable_cdn_for_container" })

@Kami
Copy link
Member

Kami commented Dec 29, 2014

This appears to be the same bug as the one introduced in #926 and later fixed #927, but this time it's actions and not sensors.

@Kami
Copy link
Member

Kami commented Dec 29, 2014

I also need to have a look how my tests which I've added recently didn't catch that. They should catch this problem :/

Lakshmi Kannan and others added 8 commits December 29, 2014 20:19
…nd update

st2ctl reload command to pass --register-sensors --register-actions flags to the
command if user doesn't specify which resource to register.
* A runner param can be declared immutable in action. In this case,
we can use the 'default' value supplied in either runner or action.
We need default to be specified in at least one place.
* Default value for a runner param made immutable in either
runner or action can also come from either.
lakshmi-kannan added a commit that referenced this pull request Dec 30, 2014
…ation

RFR: Fix action registration validation
@lakshmi-kannan lakshmi-kannan merged commit b048f02 into master Dec 30, 2014
@lakshmi-kannan lakshmi-kannan deleted the fix_action_registration_validation branch December 30, 2014 00:09
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.

3 participants