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

Rewrite wrapper script #94

Merged
merged 1 commit into from
Jan 7, 2018
Merged

Conversation

laurelmay
Copy link
Member

@laurelmay laurelmay commented Dec 24, 2017

Rewrite the Ansible wrapper script in Python to allow the user to select
which courses they need to have their machine configured for. Enable
logging by specifying a path in ansible.cfg. Remove previous wrappers
and installed desktop shortcuts in favor of the new wrapper

Resolves #93
Resolves #92

Closes #62

@laurelmay laurelmay force-pushed the use-python-wrapper branch 8 times, most recently from e298121 to 5c90e7f Compare December 25, 2017 06:47
Copy link
Member

@ripleymj ripleymj left a comment

Choose a reason for hiding this comment

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

Can you add an extra space somewhere between line 123 and 124 in the wrapper?

@laurelmay
Copy link
Member Author

laurelmay commented Dec 25, 2017

@ripleymj I just force pushed a change to fix that

ripleymj
ripleymj previously approved these changes Dec 25, 2017
@ripleymj
Copy link
Member

I think this works well enough to merge, but I'm going to hold off until later today when I can take one more look at the whole thing.

@laurelmay
Copy link
Member Author

@ripleymj I pushed a change to rebase this on top of master to avoid merge conflicts

@laurelmay laurelmay force-pushed the use-python-wrapper branch 3 times, most recently from b345533 to 90123dc Compare December 25, 2017 18:17
@laurelmay
Copy link
Member Author

Forgot to comment but the latest version has refactored the dialog creation out to a separate method. It doesn't clean stuff up too much, but hopefully it makes the dialogs slightly more uniform

ripleymj
ripleymj previously approved these changes Dec 26, 2017
@ripleymj
Copy link
Member

I've approved the changes. When you're ready, please merge, create a branch, and tag beta1.

@laurelmay laurelmay added this to the Spring 2018 milestone Dec 26, 2017
@laurelmay
Copy link
Member Author

This can be merged once we have something for #84 and #65.

@ripleymj
Copy link
Member

Need to rethink the effect on #90 here

@laurelmay
Copy link
Member Author

Good point. Due to #97, #89 and #90 are no longer closed by this. I will remove them from the initial message to prevent those from being closed upon merge.

ripleymj
ripleymj previously approved these changes Dec 26, 2017
Copy link
Member

@ripleymj ripleymj left a comment

Choose a reason for hiding this comment

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

Maybe Cancel is better labeled as Exit or Quit, since you can't really revert after a run. I might also just be trying out the shed paint.

@laurelmay
Copy link
Member Author

I agree on the Cancel vs Quit

ripleymj
ripleymj previously approved these changes Dec 26, 2017
ripleymj
ripleymj previously approved these changes Dec 27, 2017
@laurelmay
Copy link
Member Author

Two more commits. One to fix an issue that may come up soon and another to make things cleaner should strings need to be updated in the future.

@laurelmay
Copy link
Member Author

Hopefully those are two of the last commits that need to be pushed (and one of the last reviews you'll need to do). When we get ready to merge, I'll rebase this whole thing on master and squash it all into one big commit. Any other changes before then I'll keep pushing as individual commits because I think they more clearly highlight what changes each time I push and make you have to re-review.

Copy link
Member

@ripleymj ripleymj left a comment

Choose a reason for hiding this comment

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

Rejecting this out of spite.

@laurelmay
Copy link
Member Author

Once all other remaining PRs are merged and we get a resolution to #84 I think we can go ahead and merge this monster as well

ripleymj
ripleymj previously approved these changes Dec 28, 2017
@laurelmay
Copy link
Member Author

This has been rebased on top of master and all the commits have been squashed.

ripleymj
ripleymj previously approved these changes Dec 28, 2017
Rewrite the Ansible wrapper script in Python to allow the user to select
which courses they need to have their machine configured for. Enable
logging by specifying a path in ansible.cfg. Remove previous wrappers
and installed desktop shortcuts in favor of the new wrapper
@ripleymj ripleymj merged commit 66524f4 into jmunixusers:master Jan 7, 2018
@laurelmay laurelmay deleted the use-python-wrapper branch January 7, 2018 13:35
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.

2 participants