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

Fixes #19555 - Add a setting to bypass template locks #52

Merged
merged 1 commit into from May 29, 2017

Conversation

xprazak2
Copy link
Contributor

@xprazak2 xprazak2 commented May 17, 2017

I created a new setting that allows bypassing the template lock, default value is false (do not update locked templates). It can be changed in the same way as any other setting for foreman_templates.

I managed to improve messages a bit, users will know if template was skipped because it was locked. There is also something like:

template.errors if template.errors.any?

in the output, which is not very nice, but at least gives a hint what went wrong.

When executing import as non-admin user in an organization, user's org must be specified. Otherwise, there is Invalid organizations selection, you must select at least one of yours error. When template has orgs in metadata that the user is not assigned to, they are ignored - template is associated only with user's organization. Same goes for locations.

When there is OrgA with template some_tmpl and b_user (user only in OrgB) tries to import some_tmpl (which is not yet present in OrgB), it failes with name has already been taken. This is a general problem with taxonomies - some_tmpl is not found for update in scope of OrgB and when we try to create it, we hit the unique constraint on name that is not scoped by taxonomy.

I also managed to split the import! method a bit, but there is still a lot of duplicated code. I am not sure if further refactoring is worth the effort since we need to change the error handling and feedback to user.

TODO: tests

@theforeman-bot
Copy link
Member

@xprazak2, this pull request is currently not mergeable. Please rebase against the master branch and push again.

If you have a remote called 'upstream' that points to this repository, you can do this by running:

    $ git pull --rebase upstream master

This message was auto-generated by Foreman's prprocessor

Copy link
Member

@ares ares left a comment

Choose a reason for hiding this comment

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

Few comments that should be resolved before I start testing.

@@ -19,6 +19,7 @@ def import
filter: params['filter'],
associate: params['associate'],
negate: params['negate'],
force: params['force']
Copy link
Member

Choose a reason for hiding this comment

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

I think this needs apipie documentation

@@ -3,9 +3,9 @@ module ProvisioningTemplateImport
extend ActiveSupport::Concern

module ClassMethods
def import!(name, text, metadata)
def import!(name, text, metadata, force)
Copy link
Member

Choose a reason for hiding this comment

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

I'm afraid that changing this API makes importing of JobTemplates incompatbile again. If that's the case, could you please also prepare a PR for rex (and openscap seed)? We could maybe make force argument optional, defaulting to false which would avoid the need for openscap seed change.

if @verbose.is_a?(String)
@verbose = @verbose != 'false'
end
parse_bools('@verbose', '@force')
Copy link
Member

Choose a reason for hiding this comment

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

I'm happy you extracted the logic to method but I'd prefer if you remained explicit about variables and avoid using instance_variable_set as it's hard to read, how about

@verbose = parse_bool(@verbose)
@force  = parse_bool(@force)

@xprazak2
Copy link
Contributor Author

I added missing apipie doc, removed instance_variable_set and we are using default value for force in import! method

@ares
Copy link
Member

ares commented May 25, 2017

rubocop seems to be very sad :-(

@xprazak2
Copy link
Contributor Author

[test]

@ares
Copy link
Member

ares commented May 29, 2017

Works fine, local tests run seems OK.

Run options: --seed 22260

# Running:

............................................................................

Finished in 2.958844s, 25.6857 runs/s, 57.7928 assertions/s.

76 runs, 171 assertions, 0 failures, 0 errors, 0 skips

It would be great covering this in the plugin manual. Ideally the description should contain

  • an example of output that user gets when template is locked
  • explanation of force parameter
  • how organizations/locations mapping works (similar to what you described here)
  • ideally a new chapter "Common problems" or FAQ where we could add more common error messages, first candidate would be name has already been taken where the taxonomies issue would be explained.

The PR itself is ready for merge.

@xprazak2
Copy link
Contributor Author

docs

@ares
Copy link
Member

ares commented May 29, 2017

Thanks @xprazak2, merging.

@ares ares merged commit b481eb3 into theforeman:master May 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants