Skip to content
This repository has been archived by the owner on Aug 29, 2018. It is now read-only.

Fix for BZ816763 #10

Merged
merged 6 commits into from May 1, 2012
Merged

Fix for BZ816763 #10

merged 6 commits into from May 1, 2012

Conversation

thefotios
Copy link
Contributor

https://bugzilla.redhat.com/show_bug.cgi?id=816763

  • Improved ~/.openshift/express.conf generation/handling

See commit msg for more

Fotios Lindiakos added 2 commits April 27, 2012 15:14
Improved config file generation for https://bugzilla.redhat.com/show_…
10d8e2b 
…bug.cgi?id=816763
Improved ~/.openshift/express.conf generation
073db1d 
  - allows us to specify a hash of config variables, comments, and default values
  - checks the users current configuration
    - preserves modified settings
    - restores/updates comments (in case we change something)
    - adds new variables and removes deprecated ones
    - saves the user's old config to ~/.openshift/express.bak
In response to https://bugzilla.redhat.com/show_bug.cgi?id=816763

Improved handling of debug value from express.conf
  - Value was always overwritten by command line (or lack thereof)
  - This may affect other options
    - TODO: clean this up by parsing command line options into another ParseConfig object for get_var
@smarterclayton
Copy link
Contributor

One more thing - any platform specific issues possible here? Path aggregation?

@thefotios
Copy link
Contributor Author

There shouldn't be. It uses mostly the same functions we've been using to
get and set the config and the same path config vars. I'll check out the
file body too and push a fix. Can I update this pull req or do I have to
close it and make a new one?
On Apr 28, 2012 2:40 PM, "Clayton Coleman" <
reply@reply.github.com>
wrote:

One more thing - any platform specific issues possible here? Path
aggregation?

Reply to this email directly or view it on GitHub:
https://github.com/openshift/os-client-tools/pull/10#issuecomment-5399048

@smarterclayton
Copy link
Contributor

Just update the one. I'm just being a bit more cautious on changes
cause of the issues on Friday - we'll all talk on Monday but there
will be a bigger focus on testing client installer and validating
across platforms for both test and dev.

On Apr 28, 2012, at 3:03 PM, Fotios Lindiakos
reply@reply.github.com
wrote:

There shouldn't be. It uses mostly the same functions we've been using to
get and set the config and the same path config vars. I'll check out the
file body too and push a fix. Can I update this pull req or do I have to
close it and make a new one?
On Apr 28, 2012 2:40 PM, "Clayton Coleman" <
reply@reply.github.com>
wrote:

One more thing - any platform specific issues possible here? Path
aggregation?

Reply to this email directly or view it on GitHub:
https://github.com/openshift/os-client-tools/pull/10#issuecomment-5399048

Reply to this email directly or view it on GitHub:
https://github.com/openshift/os-client-tools/pull/10#issuecomment-5399250

@thefotios
Copy link
Contributor Author

Think I've gotten everything nailed down. I separated most of the functionality into different functions. Then I wrote tests for all of them trying to cover as many scenarios as I could think of.

The tests can be run via rake test (I've also renamed the REST tests so that they don't get run, since they hang unless pointed to an active devenv)

@smarterclayton
Copy link
Contributor

Will try tonight - thanks for taking and pushing this

On Mon, Apr 30, 2012 at 2:37 PM, Fotios Lindiakos <
reply@reply.github.com

wrote:

Think I've gotten everything nailed down. I separated most of the
functionality into different functions. Then I wrote tests for all of them
trying to cover as many scenarios as I can.

The tests can be run via rake test (I've also renamed the REST tests so
that they don't get run, since they hang unless pointed to an active devenv)

Reply to this email directly or view it on GitHub:
https://github.com/openshift/os-client-tools/pull/10#issuecomment-5424306

smarterclayton added a commit that referenced this pull request May 1, 2012
@smarterclayton
Merge pull request #10 from fotioslindiakos/config_file
d27b396 
Fix for BZ816763
@smarterclayton smarterclayton merged commit d27b396 into openshift:master May 1, 2012
smarterclayton added a commit that referenced this pull request May 1, 2012
@smarterclayton
Revert "Merge pull request #10 from fotioslindiakos/config_file" - so…
97036cb 
…me failing tests, will retry. Be sure to resubmit pull request.

This reverts commit d27b396, reversing
changes made to 169cd1d.
@smarterclayton
Copy link
Contributor

Ended up needing to revert this because of test breaks. The other tests in core are breaking with this changeset. I think you'll need to recreate the pull request with the fixes on top (you may need new changesets as well by creating a patch from the range of changesets).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@thefotios @smarterclayton