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

new crowbar_batch script #1147

Merged
merged 14 commits into from Nov 10, 2014

Conversation

3 participants
@aspiers
Member

aspiers commented Oct 16, 2014

(see #1152 for backport to release/stoney/master)

crowbar batch build foo.yaml

will automatically create (if necessary), edit, and apply all proposals described within foo.yaml. This enables building arbitrary cloud configurations in an entirely automated fashion, which can be very useful for testing, building reproducible demos, and eventually maybe even partially mimicking customer environments.

@aspiers aspiers changed the title from [WIP] new crowbar_autobuild batch script to new crowbar_autobuild batch script Oct 21, 2014

@aspiers

This comment has been minimized.

Show comment
Hide comment
@aspiers

aspiers Oct 21, 2014

Member

This is not yet perfect, but I think it's probably good enough to merge (and very low risk, since it mainly just adds one new file).

Member

aspiers commented Oct 21, 2014

This is not yet perfect, but I think it's probably good enough to merge (and very low risk, since it mainly just adds one new file).

@aspiers aspiers changed the title from new crowbar_autobuild batch script to new crowbar_batch script Oct 21, 2014

Show outdated Hide outdated bin/crowbar_batch
Show outdated Hide outdated bin/crowbar_batch
@tboerger

This comment has been minimized.

Show comment
Hide comment
@tboerger

tboerger Oct 22, 2014

Contributor

I have not tested that but looks mostly fine

Contributor

tboerger commented Oct 22, 2014

I have not tested that but looks mostly fine

@aspiers

This comment has been minimized.

Show comment
Hide comment
@aspiers

aspiers Oct 22, 2014

Member

Thanks - have fixed the issues you found.

Member

aspiers commented Oct 22, 2014

Thanks - have fixed the issues you found.

@tboerger

This comment has been minimized.

Show comment
Hide comment
@tboerger

tboerger Oct 22, 2014

Contributor

👍 Hopefully we will get the export in another pull request?! :)

Contributor

tboerger commented Oct 22, 2014

👍 Hopefully we will get the export in another pull request?! :)

@aspiers

This comment has been minimized.

Show comment
Hide comment
@aspiers

aspiers Oct 22, 2014

Member

Yes exactly :)

Member

aspiers commented Oct 22, 2014

Yes exactly :)

@aspiers

This comment has been minimized.

Show comment
Hide comment
@aspiers

aspiers Oct 22, 2014

Member

@bkutil Any chance you could review and merge?

Member

aspiers commented Oct 22, 2014

@bkutil Any chance you could review and merge?

@aspiers

This comment has been minimized.

Show comment
Hide comment
@aspiers

aspiers Oct 23, 2014

Member

@tboerger Export is now in this PR :)

Member

aspiers commented Oct 23, 2014

@tboerger Export is now in this PR :)

aspiers added a commit to aspiers/suse-cloud-vagrant that referenced this pull request Oct 23, 2014

add crowbar_batch
This is not in the product yet, but hopefully will be soon:

  crowbar/barclamp-crowbar#1147
@bkutil

This comment has been minimized.

Show comment
Hide comment
@bkutil

bkutil Oct 23, 2014

Contributor

@aspiers I just looked at the code briefly, but in general:

  • method names suggest common theme (e.g. Proposal), would it be possible to use classes?
  • implementation details are mixed with core functionality, so maybe splitting 'ugly hacks and 1.8.7 workarounds' from 'modify proposal' (e.g. using an Utils class)?

Maybe that more stuff (e.g. consistent response handling, common code for API consumption, logging) will arise if the above is accomplished.

I know this is just 'another script' in the suite (which already is a mess), but from what you say, there are plans to extend it, so this will be read/modified many times and the time investment will pay off, IMHO.

Contributor

bkutil commented Oct 23, 2014

@aspiers I just looked at the code briefly, but in general:

  • method names suggest common theme (e.g. Proposal), would it be possible to use classes?
  • implementation details are mixed with core functionality, so maybe splitting 'ugly hacks and 1.8.7 workarounds' from 'modify proposal' (e.g. using an Utils class)?

Maybe that more stuff (e.g. consistent response handling, common code for API consumption, logging) will arise if the above is accomplished.

I know this is just 'another script' in the suite (which already is a mess), but from what you say, there are plans to extend it, so this will be read/modified many times and the time investment will pay off, IMHO.

Show outdated Hide outdated bin/crowbar_batch
@aspiers

This comment has been minimized.

Show comment
Hide comment
@aspiers

aspiers Oct 23, 2014

Member

@bkutil wrote:

@aspiers I just looked at the code briefly, but in general:

  • method names suggest common theme (e.g. Proposal), would it be possible to use classes?

Sure.

  • implementation details are mixed with core functionality, so maybe splitting 'ugly hacks and 1.8.7
    workarounds' from 'modify proposal'?

Not sure I follow - the only 1.8.7 workaround I know of isn't in modify_proposal? The ugly hack in
there is unavoidable without doing a huge refactoring of the whole of /opt/dell/bin, and I simply
don't have time for that right now.

Maybe that more stuff (e.g. consistent response handling, common code for API consumption, logging)
will arise if the above is accomplished.

Depends on what you mean by "the above". Classes would definitely help, for sure. But this is a major
change spanning all the barclamps, so it would need to be done as a full-blown sprint story, not as
part of this PR.

I know this is just 'another script' in the suite (which already is a mess), but from what you say,
there are plans to extend it

I don't have plans to extend it. How did I mistakenly give you that idea?

so this will be read/modified many times and the time investment will pay off, IMHO.

Actually I view this particular script as more or less "done" already. It does two simple things pretty
well. OK, there are some simple enhancements I could make, e.g. supporting parameters and allowing
include/exclude of particular barclamps in build mode, but that's about it.

Member

aspiers commented Oct 23, 2014

@bkutil wrote:

@aspiers I just looked at the code briefly, but in general:

  • method names suggest common theme (e.g. Proposal), would it be possible to use classes?

Sure.

  • implementation details are mixed with core functionality, so maybe splitting 'ugly hacks and 1.8.7
    workarounds' from 'modify proposal'?

Not sure I follow - the only 1.8.7 workaround I know of isn't in modify_proposal? The ugly hack in
there is unavoidable without doing a huge refactoring of the whole of /opt/dell/bin, and I simply
don't have time for that right now.

Maybe that more stuff (e.g. consistent response handling, common code for API consumption, logging)
will arise if the above is accomplished.

Depends on what you mean by "the above". Classes would definitely help, for sure. But this is a major
change spanning all the barclamps, so it would need to be done as a full-blown sprint story, not as
part of this PR.

I know this is just 'another script' in the suite (which already is a mess), but from what you say,
there are plans to extend it

I don't have plans to extend it. How did I mistakenly give you that idea?

so this will be read/modified many times and the time investment will pay off, IMHO.

Actually I view this particular script as more or less "done" already. It does two simple things pretty
well. OK, there are some simple enhancements I could make, e.g. supporting parameters and allowing
include/exclude of particular barclamps in build mode, but that's about it.

@bkutil

This comment has been minimized.

Show comment
Hide comment
@bkutil

bkutil Oct 24, 2014

Contributor

Not sure I follow - the only 1.8.7 workaround I know of isn't in modify_proposal?

Yeah, sorry, this was a bad example. The problem I was trying to point out was, that the workaround-methods are mixed with the other ones (functionality vs. low level noise), not that they call / contain each other.

Maybe that more stuff (e.g. consistent response handling, common code for API consumption, logging) will arise if the above is accomplished.
Depends on what you mean by "the above". Classes would definitely help, for sure. But this is major change spanning all the barclamps, so it would need to be done as a full-blown sprint story, not as part of this PR.

This can be done just for this script. From that, it'll be more clear what is required for the other scripts and the lib. Also, it'd be a good example how adding just some encapsulation / logical grouping can make the other scripts so much better. Process-wise, the bigger refactoring can be handled however you see fit and can be done by someone else.

I know this is just 'another script' in the suite (which already is a mess), but from what you say,
there are plans to extend it
I don't have plans to extend it. How did I mistakenly give you that idea?

Initially, I assumed that 'and eventually maybe even partially mimicking customer environments.' would mean this would need to be extended or modified somehow. And your commit 779e326 seems to suggests that as well - or do I get that wrong?

Actually I view this particular script as more or less "done" already. It does two simple things pretty
well.

Yes, this is functionality wise. But in order to review it, and make sense of it just by looking at the code, you have to parse 500 lines of unstructured stuff - basically just a long list of functions that do 'something'. Adding some structure, however crude, will help a lot, imho.

Contributor

bkutil commented Oct 24, 2014

Not sure I follow - the only 1.8.7 workaround I know of isn't in modify_proposal?

Yeah, sorry, this was a bad example. The problem I was trying to point out was, that the workaround-methods are mixed with the other ones (functionality vs. low level noise), not that they call / contain each other.

Maybe that more stuff (e.g. consistent response handling, common code for API consumption, logging) will arise if the above is accomplished.
Depends on what you mean by "the above". Classes would definitely help, for sure. But this is major change spanning all the barclamps, so it would need to be done as a full-blown sprint story, not as part of this PR.

This can be done just for this script. From that, it'll be more clear what is required for the other scripts and the lib. Also, it'd be a good example how adding just some encapsulation / logical grouping can make the other scripts so much better. Process-wise, the bigger refactoring can be handled however you see fit and can be done by someone else.

I know this is just 'another script' in the suite (which already is a mess), but from what you say,
there are plans to extend it
I don't have plans to extend it. How did I mistakenly give you that idea?

Initially, I assumed that 'and eventually maybe even partially mimicking customer environments.' would mean this would need to be extended or modified somehow. And your commit 779e326 seems to suggests that as well - or do I get that wrong?

Actually I view this particular script as more or less "done" already. It does two simple things pretty
well.

Yes, this is functionality wise. But in order to review it, and make sense of it just by looking at the code, you have to parse 500 lines of unstructured stuff - basically just a long list of functions that do 'something'. Adding some structure, however crude, will help a lot, imho.

@aspiers

This comment has been minimized.

Show comment
Hide comment
@aspiers

aspiers Oct 26, 2014

Member

Balazs Kutil notifications@github.com wrote:

Not sure I follow - the only 1.8.7 workaround I know of isn't in modify_proposal?

Yeah, sorry, this was a bad example. The problem I was trying to point out was, that the workaround-methods are mixed with the other ones (functionality vs. low level noise), not that they call / contain each other.

I don't see how else to do it without also doing a major refactoring of barclamp_lib.rb, which would in turn require a major refactoring of all the crowbar_* commands across all the barclamps. And like I already said this would require a significant sprint story, and there's no way I'll have time to do this in the next few weeks, even though I need crowbar_batch for Paris and SUSEcon.

Maybe that more stuff (e.g. consistent response handling, common code for API consumption, logging) will arise if the above is accomplished.
Depends on what you mean by "the above". Classes would definitely help, for sure. But this is major change spanning all the barclamps, so it would need to be done as a full-blown sprint story, not as part of this PR.

This can be done just for this script.

How? I really don't think it can, because the API calls are inextricably interwined with barclamp_lib.rb. But if you're convinced it can, feel free to show what I'm missing.

From that, it'll be more clear what is required for the other scripts and the lib.

It's already fairly clear to me, I just don't have time to do it yet.

Also, it'd be a good example how adding just some encapsulation / logical grouping can make the other scripts so much better.

Happy to if it's quick and simple - concrete suggestions welcome.

Process-wise, the bigger refactoring can be handled however you see fit and can be done by someone else.

Yep.

I know this is just 'another script' in the suite (which already is a mess), but from what you say,
there are plans to extend it
I don't have plans to extend it. How did I mistakenly give you that idea?

Initially, I assumed that 'and eventually maybe even partially mimicking customer environments.' would mean this would need to be extended or modified somehow.

It can already do that. Just crowbar batch export on the customer cloud and then crowbar batch build on a test cloud.

And your commit 779e326 seems to suggests that as well - or do I get that wrong?

Now you're cheating because I added that TODO list after you guessed there were plans to extend it ;-) But really they are just three minor tweaks rather than major extensions.

Actually I view this particular script as more or less "done" already. It does two simple things pretty
well.

Yes, this is functionality wise. But in order to review it, and make sense of it just by looking at the code, this is 350 lines of unstructured stuff - just a long list of functions that do 'something'. Adding some structure, however crude, will help a lot, imho.

Like I said, I'm not really sure what you mean but I'm happy to do quick and simple small changes if you can suggest them. But I'm already flat out preparing for Paris and SUSEcon, so bigger changes will have to wait.

Member

aspiers commented Oct 26, 2014

Balazs Kutil notifications@github.com wrote:

Not sure I follow - the only 1.8.7 workaround I know of isn't in modify_proposal?

Yeah, sorry, this was a bad example. The problem I was trying to point out was, that the workaround-methods are mixed with the other ones (functionality vs. low level noise), not that they call / contain each other.

I don't see how else to do it without also doing a major refactoring of barclamp_lib.rb, which would in turn require a major refactoring of all the crowbar_* commands across all the barclamps. And like I already said this would require a significant sprint story, and there's no way I'll have time to do this in the next few weeks, even though I need crowbar_batch for Paris and SUSEcon.

Maybe that more stuff (e.g. consistent response handling, common code for API consumption, logging) will arise if the above is accomplished.
Depends on what you mean by "the above". Classes would definitely help, for sure. But this is major change spanning all the barclamps, so it would need to be done as a full-blown sprint story, not as part of this PR.

This can be done just for this script.

How? I really don't think it can, because the API calls are inextricably interwined with barclamp_lib.rb. But if you're convinced it can, feel free to show what I'm missing.

From that, it'll be more clear what is required for the other scripts and the lib.

It's already fairly clear to me, I just don't have time to do it yet.

Also, it'd be a good example how adding just some encapsulation / logical grouping can make the other scripts so much better.

Happy to if it's quick and simple - concrete suggestions welcome.

Process-wise, the bigger refactoring can be handled however you see fit and can be done by someone else.

Yep.

I know this is just 'another script' in the suite (which already is a mess), but from what you say,
there are plans to extend it
I don't have plans to extend it. How did I mistakenly give you that idea?

Initially, I assumed that 'and eventually maybe even partially mimicking customer environments.' would mean this would need to be extended or modified somehow.

It can already do that. Just crowbar batch export on the customer cloud and then crowbar batch build on a test cloud.

And your commit 779e326 seems to suggests that as well - or do I get that wrong?

Now you're cheating because I added that TODO list after you guessed there were plans to extend it ;-) But really they are just three minor tweaks rather than major extensions.

Actually I view this particular script as more or less "done" already. It does two simple things pretty
well.

Yes, this is functionality wise. But in order to review it, and make sense of it just by looking at the code, this is 350 lines of unstructured stuff - just a long list of functions that do 'something'. Adding some structure, however crude, will help a lot, imho.

Like I said, I'm not really sure what you mean but I'm happy to do quick and simple small changes if you can suggest them. But I'm already flat out preparing for Paris and SUSEcon, so bigger changes will have to wait.

aspiers added some commits Oct 16, 2014

new crowbar_batch script
  crowbar batch build foo.yaml

will automatically create (if necessary), edit, and apply all proposals
described within foo.yaml.  This enables building arbitrary cloud
configurations in an entirely automated fashion, which can be very
useful for testing, building reproducible demos, and eventually maybe
even partially mimicking customer environments.
@aspiers

This comment has been minimized.

Show comment
Hide comment
@aspiers

aspiers Oct 26, 2014

Member

I've squashed a load of commits to make reviewing easier, and also added some other improvements.

Member

aspiers commented Oct 26, 2014

I've squashed a load of commits to make reviewing easier, and also added some other improvements.

@aspiers

This comment has been minimized.

Show comment
Hide comment
@aspiers

aspiers Nov 10, 2014

Member

@bkutil, @tboerger: this worked nicely in Paris and is really useful so I want to get it merged into master and release/stoney/master ASAP.

Member

aspiers commented Nov 10, 2014

@bkutil, @tboerger: this worked nicely in Paris and is really useful so I want to get it merged into master and release/stoney/master ASAP.

@tboerger

This comment has been minimized.

Show comment
Hide comment
@tboerger

tboerger Nov 10, 2014

Contributor

As already told on IRC we don't like the current style but it is the overall style for the CLI. We need to refactor everythiong of it but it doesnt make sense to mix the approaches. So for now you get +2 from my side :)

Contributor

tboerger commented Nov 10, 2014

As already told on IRC we don't like the current style but it is the overall style for the CLI. We need to refactor everythiong of it but it doesnt make sense to mix the approaches. So for now you get +2 from my side :)

@aspiers

This comment has been minimized.

Show comment
Hide comment
@aspiers

aspiers Nov 10, 2014

Member

Thanks. Trello card for technical debt has been filed.

Member

aspiers commented Nov 10, 2014

Thanks. Trello card for technical debt has been filed.

@aspiers

This comment has been minimized.

Show comment
Hide comment
@aspiers

aspiers Nov 10, 2014

Member

Merging as discussed on #cloud with @bkutil and @tboerger.

Member

aspiers commented Nov 10, 2014

Merging as discussed on #cloud with @bkutil and @tboerger.

aspiers added a commit that referenced this pull request Nov 10, 2014

@aspiers aspiers merged commit ff82797 into crowbar:master Nov 10, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@aspiers aspiers deleted the aspiers:autobuild branch Nov 10, 2014

@aspiers aspiers referenced this pull request Nov 10, 2014

Closed

new crowbar_batch script #1152

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment