Fixes #17417 - adds exporting task #36

ares · 2016-11-21T16:40:29Z

Requires theforeman/foreman#4031 but other than that, ready for review. I didn't cover it in readme since I'd like to write a proper manual for the plugin and start linking there. If you want to test, run rake templates:export. Just be careful, currently it clones remote repo, does changes, creates commit and pushes to the origin. After discussion with @xprazak2 we decided to leave it like this until there's a filesystem support (#35). Later we'd like to introduce repos as first class citizen object that users could configure separately.

GregSutcliffe

Mostly 👍 just a few nitpicks. I'll set it up now and try it out :)

GregSutcliffe · 2016-11-23T15:06:31Z

app/models/setting/template_sync.rb

  def self.load_defaults
    return unless super

-    %w(template_sync_filter template_sync_branch).each { |s| Setting::BLANK_ATTRS << s }
+    %w(template_sync_filter template_sync_branch ).each { |s| Setting::BLANK_ATTRS << s }


Nitpick: is this extra whitespace intentional?

GregSutcliffe · 2016-11-23T15:08:55Z

app/models/setting/template_sync.rb

        self.set('template_sync_repo', N_('Default Git repo to sync from'), 'https://github.com/theforeman/community-templates.git', N_('Repo')),
-        self.set('template_sync_negate', N_('Negate the search'), false, N_('Negate')),
-        self.set('template_sync_branch', N_('Default branch in Git repo'), nil, N_('Branch'))
+        self.set('template_sync_negate', N_('Negate the prefix search for purging, filter for importing or exporting'), false, N_('Negate')),


Nitpick: This can probably be worded better - perhaps "Negate the prefix (for purging) / filter (for importing/exporting)" ?

GregSutcliffe · 2016-11-23T15:23:02Z

lib/tasks/foreman_templates_tasks.rake

@@ -24,6 +27,23 @@ namespace :templates do
    puts results.join("\n")
  end

+  task :import => :sync


Nitpick: Shouldn't this the other way around? :sync is deprecated, so the code should now be at :import

GregSutcliffe · 2016-11-23T15:25:01Z

app/services/foreman_templates/template_exporter.rb

+      end
+    end
+
+    def get_template_name(template)


name is a bit overloaded, using get_template_filename would be clearer

GregSutcliffe · 2016-11-23T15:27:29Z

lib/tasks/foreman_templates_tasks.rake

+      metadata_export_mode: ENV['metadata_export_mode'],
+    }).export!
+
+    puts 'Export finished'


I see the internal code logs using logger - but just this single line for rake. Can we get output parity here (at least using @verbose)?

Ignore me, seems to work just fine now that I try it ;)

GregSutcliffe · 2016-11-23T15:56:59Z

@ares: Should this work with local git dirs? I tried this:

mkdir /tmp/foo && cd /tmp/foo && git init && cd - 
bundle exec rake templates:export verbose=true repo="/tmp/foo"

and it fails with

...
2016-11-23T15:55:35  [app] [D] Writing to file /tmp/d20161123-9598-2oioni/provisioning_templates/PXELinux/Community greg_alterator_default_pxelinux.erb
2016-11-23T15:55:35  [app] [D] finished writing 640
2016-11-23T15:55:35  [app] [D] commiting changes in cloned repo
2016-11-23T15:55:35  [app] [D] pushing to branch  at origin /tmp/foo
rake aborted!
Git::GitExecuteError: git '--git-dir=/tmp/d20161123-9598-2oioni/.git' '--work-tree=/tmp/d20161123-9598-2oioni' push 'origin' ''  2>&1:fatal: remote part of refspec is not a valid name in 
/home/greg/github/foreman_templates/app/services/foreman_templates/template_exporter.rb:21:in `export!'
/home/greg/github/foreman_templates/lib/tasks/foreman_templates_tasks.rake:42:in `block (2 levels) in <top (required)>'
/home/greg/.rbenv/versions/2.3.1/bin/bundle:23:in `load'
/home/greg/.rbenv/versions/2.3.1/bin/bundle:23:in `<main>'
Tasks: TOP => templates:export
(See full trace by running task with --trace)

ares · 2016-11-23T16:03:09Z

It should, you need to have at least one commit there though. Also note that the branch must not be checked out before pushing (e.g. checkout to different branch than you push to).

GregSutcliffe · 2016-11-23T16:07:46Z

No, it still bombs. I'm doing this:

rm -rf /tmp/foo ; mkdir /tmp/foo ; cd /tmp/foo
git init ; touch foo ; git add foo ; git ci -m "Init" ; git co -t master -b devel ; cd -
Initialized empty Git repository in /tmp/foo/.git/
bundle exec rake templates:export verbose=true repo="/tmp/foo" prefix="Greg "

Same error, I think:

2016-11-23T16:05:08  [app] [D] commiting changes in cloned repo
2016-11-23T16:05:08  [app] [D] pushing to branch  at origin /tmp/foo
rake aborted!
Git::GitExecuteError: git '--git-dir=/tmp/d20161123-10907-176cv2s/.git' '--work-tree=/tmp/d20161123-10907-176cv2s' push 'origin' ''  2>&1:fatal: remote part of refspec is not a valid name in 
/home/greg/github/foreman_templates/app/services/foreman_templates/template_exporter.rb:21:in `export!'
/home/greg/github/foreman_templates/lib/tasks/foreman_templates_tasks.rake:42:in `block (2 levels) in <top (required)>'

GregSutcliffe · 2016-11-23T16:09:16Z

The prefixing is a bit weird too. I used templates:import prefix='Greg ' to import my templates, then when I export with the same prefix, I get:

/tmp/d20161123-9799-1y90ler/provisioning_templates/PXELinux/Greg greg_alterator_default_pxelinux.erb

See how it's kept the import prefix (but with lowercase and whitespace->underscore) but also added the prefix again. This is also not handled in the name: field of the dumped text (it's Greg Alterator ...)

The filename is mostly cosmetic, but (assuming you automate the import step with a known prefix) the name metadata would lead to an increasing number of prefixes on each import/export cycle, since "Greg Alterator..." would get prefixed with "Greg " again on the second import.

Both import and export should probably check if the string already starts with the prefix and drop it if so.

ares · 2016-12-07T12:37:27Z

@GregSutcliffe the error you're receiving is caused by the fact you haven't created a development branch in your repo. We default to it since we assume the repo is community-templates. You can also override the branch in settings but it must exist in target repo. We should probably add some layer for better error detection and reporting, but that's out of scope for this PR I'm afraid as this is the same for importing.

Regarding the prefix, I think we just need to stop importing double prefixes. During the export, prefix is already part of the name and you're right, it does not make sense to add it to the filename since normally it should already be there.

I fixed all your comments, please take another look. I'd also like to add pure FS export support in this PR so please do not merge yet. I hope to find some time today to finish it.

ares · 2016-12-07T17:11:06Z

Now with FS support, so cancelling the "do not merge" status :-)

sean797 · 2016-12-09T11:38:32Z

lib/tasks/foreman_templates_tasks.rake

-  task :sync => :environment do
+  task :import => :environment do
+    if Rake.application.top_level_tasks.include?('templates:sync')
+      ActiveSupport::Deprecation.warn('templates:sync task has been renamed to templates:import and will be removed in future version')


in a future version ?

GregSutcliffe · 2016-12-09T15:05:57Z

@ares we're closer but not done yet :)

The git thing makes sense - I've tested with a real repo and it seems good. For reference I used bundle exec rake templates:export verbose=true repo="https://github.com/GregSutcliffe/community-templates.git" branch="disk" prefix="Greg " and it resulted in https://github.com/GregSutcliffe/community-templates/tree/disk
File based export is working fine too 🎉
Prefixes are still broken

So you've only added auto_prefix to import as far as I can tell. But you can still specify it on export (as in my example above). So if you start with a template called "foo_bar" it exports as "Greg foo_bar" and then imports as "Greg foo_bar". After one round it seems to stabilise - I don't see "Greg Greg greg_foo_bar" or anything like that - but it's still a bit weird.

Does it make sense to drop the prefix option from the export task? We're just trying to represent the state of the Foreman DB on disk, so perhaps we shouldn't prefix the names.

There's also two other small things I'll comment on inline.

EDIT: there is still some weirdness on export/import with prefixes - after a few rounds I see this in my export dir:

    ├── PXELinux
    │   ├── Greg alterator_default_pxelinux.erb
    │   ├── Greg greg_alterator_default_pxelinux.erb

Note how the original alterator... template has been exported as Greg alterator... and then the prefix has been duplicated again the second time. I don't see any "Greg Greg Alterator" templates in the UI though, so it's better than it was :)

GregSutcliffe · 2016-12-09T15:07:51Z

app/services/foreman_templates/template_importer.rb

+          matching = name.match(/#{@filter}/i)
+          matching = !matching if @negate
+          next if !matching
+        end

        raise MissingKindError unless metadata['kind']


The export task isn't writing kind for ptables or snippets, so the subsequent import fails. We should probably remove this check now that we're moving to model - but we might want to add model to the export task so that we always hit line 79 below

GregSutcliffe · 2016-12-09T15:09:40Z

app/services/foreman_templates/template_exporter.rb

+      git_repo = Git.clone(@repo, @dir)
+      logger.debug "cloned #{@repo} to #{@dir}"
+      branch = @branch ? @branch : get_default_branch(git_repo)
+      git_repo.checkout(branch) if branch


Is there any chance to create the branch if we specify a non-existant one? Currently it fails unless you give a valid branch, and it felt a bit weird to have to clone my repo separately just to add a target for exporting. This is not a blocker and can be done in another PR, just curious

easy enough to do it in this PR

ares · 2016-12-10T08:55:10Z

I kept changes in separate commit for easier reviewing. I think it should address all recent comments. Please take another look.

ares · 2016-12-13T16:39:56Z

theforeman/foreman#4098 has been merged so exported templates will have model in the yaml if it runs with 1.15

UPDATE: the PR was merged and aligned to 1.14.1 in redmine so this might work Foreman 1.14.1+

ares · 2016-12-31T17:49:53Z

There's still a chance to get this merged in 2016 :-)

GregSutcliffe · 2017-01-05T11:34:14Z

Sorry for the delay @ares

Testing pretty well here, the only minor issue/question left is what to do with prefixed imports. Currently you get something like this:

> select name, template from templates;
...
Greg Preseed default|<%#
name: Preseed default
snippet: false
model: Ptable
os_family: Debian
%>

Note how the Prefix goes in the name but not the metadata. Now, when this is exported it'll be fixed (assuming metadata==refresh) but it leaves it slightly inconsistent in the meantime.

I don't think it's a big issue, so let's [test] one more time, and then merge, and we can deal with edge cases as we go.

ares · 2017-01-05T12:29:23Z

I didn't find a better way. I agree, lets hear from users how they would prefer to solve it. If this is commonly used thing we might have prefix as first class citizen attribute in templates.

GregSutcliffe · 2017-01-05T15:18:11Z

@ares looks like you've got a minor test failure. +1 when it's green ;)

* rely on model attribute on import * drop prefix on export * create branch if it does not exist * fix deprecation message

ares · 2017-01-05T16:19:22Z

thou shalt not fail

ares · 2017-01-06T07:55:39Z

@GregSutcliffe 🍏

GregSutcliffe · 2017-01-09T20:10:04Z

Squashed and merged, ty @ares for the hard work and patience :)

theforeman-bot added Needs testing Not yet reviewed labels Nov 21, 2016

GregSutcliffe suggested changes Nov 23, 2016

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Nov 23, 2016

ares force-pushed the fix/17417 branch from 5cea416 to d6314cd Compare December 7, 2016 12:36

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Dec 7, 2016

ares added 2 commits December 7, 2016 13:37

Deprecate sync in favor of import

022cc88

Fixes #17417 - adds exporting task

2e25d74

ares force-pushed the fix/17417 branch from d6314cd to 03edfe4 Compare December 7, 2016 12:38

Stop double prefixing on import

3653c74

ares force-pushed the fix/17417 branch from 03edfe4 to 3653c74 Compare December 7, 2016 12:48

Refs #17417 - add FS support for exporting

298152e

sean797 reviewed Dec 9, 2016

View reviewed changes

GregSutcliffe reviewed Dec 9, 2016

View reviewed changes

GregSutcliffe self-assigned this Jan 5, 2017

GregSutcliffe approved these changes Jan 5, 2017

View reviewed changes

Refs #17417 - small fixes

d602f98

* rely on model attribute on import * drop prefix on export * create branch if it does not exist * fix deprecation message

ares force-pushed the fix/17417 branch from 1e05462 to d602f98 Compare January 5, 2017 16:17

GregSutcliffe merged commit b989762 into theforeman:master Jan 9, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #17417 - adds exporting task #36

Fixes #17417 - adds exporting task #36

ares commented Nov 21, 2016

GregSutcliffe left a comment

GregSutcliffe Nov 23, 2016

GregSutcliffe Nov 23, 2016

GregSutcliffe Nov 23, 2016

GregSutcliffe Nov 23, 2016

GregSutcliffe Nov 23, 2016

GregSutcliffe Nov 23, 2016

GregSutcliffe commented Nov 23, 2016

ares commented Nov 23, 2016

GregSutcliffe commented Nov 23, 2016 •

edited

GregSutcliffe commented Nov 23, 2016

ares commented Dec 7, 2016

ares commented Dec 7, 2016

sean797 Dec 9, 2016

GregSutcliffe commented Dec 9, 2016 •

edited

GregSutcliffe Dec 9, 2016

GregSutcliffe Dec 9, 2016

ares Dec 9, 2016

ares commented Dec 10, 2016

ares commented Dec 13, 2016 •

edited

ares commented Dec 31, 2016

GregSutcliffe commented Jan 5, 2017

ares commented Jan 5, 2017

GregSutcliffe commented Jan 5, 2017

ares commented Jan 5, 2017

ares commented Jan 6, 2017

GregSutcliffe commented Jan 9, 2017

Fixes #17417 - adds exporting task #36

Fixes #17417 - adds exporting task #36

Conversation

ares commented Nov 21, 2016

GregSutcliffe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GregSutcliffe commented Nov 23, 2016

ares commented Nov 23, 2016

GregSutcliffe commented Nov 23, 2016 • edited

GregSutcliffe commented Nov 23, 2016

ares commented Dec 7, 2016

ares commented Dec 7, 2016

Choose a reason for hiding this comment

GregSutcliffe commented Dec 9, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ares commented Dec 10, 2016

ares commented Dec 13, 2016 • edited

ares commented Dec 31, 2016

GregSutcliffe commented Jan 5, 2017

ares commented Jan 5, 2017

GregSutcliffe commented Jan 5, 2017

ares commented Jan 5, 2017

ares commented Jan 6, 2017

GregSutcliffe commented Jan 9, 2017

GregSutcliffe commented Nov 23, 2016 •

edited

GregSutcliffe commented Dec 9, 2016 •

edited

ares commented Dec 13, 2016 •

edited