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

Collection of patches for crmsh #77

Merged
merged 3 commits into from Jan 20, 2015
Merged

Conversation

krig
Copy link
Contributor

@krig krig commented Jan 19, 2015

No description provided.

@krig
Copy link
Contributor Author

krig commented Jan 20, 2015

Alright, with that sorted I will merge this. Thanks both of you!

krig added a commit that referenced this pull request Jan 20, 2015
Collection of patches for crmsh
@krig krig merged commit a8884dd into ClusterLabs:master Jan 20, 2015
@krig krig deleted the mail-patchset branch January 20, 2015 07:59
@krig
Copy link
Contributor Author

krig commented Jan 22, 2015

Hmm. I noticed now that the transition patch broke the history test case.

It seems no transitions are found after applying this patch:

-Transitions: 43 44 45 46 48 272 49 50
+Transitions: 
+ERROR: 23: no transitions found in the source

Is this a reason to revert the patch?

@krig krig added the bug label Jan 22, 2015
@krig krig self-assigned this Jan 22, 2015
@dmuhamedagic
Copy link
Collaborator

On Thu, Jan 22, 2015 at 01:52:27AM -0800, Kristoffer Grönlund wrote:

Hmm. I noticed now that the transition patch broke the history test case.

It seems no transitions are found after applying this patch:

-Transitions: 43 44 45 46 48 272 49 50
+Transitions: 
+ERROR: 23: no transitions found in the source

Oops, that's not good.

Is this a reason to revert the patch?

Hmm, perhaps, I don't know, will take a look.

@dmuhamedagic
Copy link
Collaborator

On Thu, Jan 22, 2015 at 01:52:27AM -0800, Kristoffer Grönlund wrote:

Hmm. I noticed now that the transition patch broke the history test case.

It seems no transitions are found after applying this patch:

-Transitions: 43 44 45 46 48 272 49 50
+Transitions: 
+ERROR: 23: no transitions found in the source

Is this a reason to revert the patch?

The message format in this patch got introduced with this change:

commit d13ce588e9fe4277d6570a54f0728223fe505627
Author: Andrew Beekhof andrew@beekhof.net
Date: Mon Sep 10 11:35:03 2012 +1000

Low: PE: Minor cleanup

...

  •        crm_err("Transition %d:"
    
  •                " ERRORs found during PE processing."
    
  •                " PEngine Input stored in: %s", transition_id, filename);
    
  •        crm_err("Calculated Transition %d: %s",
    
  •                transition_id, filename);
    
     } else if (was_processing_warning) {
    
  •        crm_warn("Transition %d:"
    
  •                 " WARNINGs found during PE processing."
    
  •                 " PEngine Input stored in: %s", transition_id, filename);
    
  •        crm_warn("Calculated Transition %d: %s",
    
  •                 transition_id, filename);
    
     } else {
    
  •        crm_notice("Transition %d: PEngine Input stored in: %s", transition
    
    _id, filename);
  •        crm_notice("Calculated Transition %d: %s",
    
  •                   transition_id, filename);
    

Maybe something like this would do:

"pengine.* process_pe_message: .*Transition ([0-9]+): .*([^ ]*/pe-[^-]+-(%%)[.]bz2)",

krig added a commit to krig/crmsh that referenced this pull request Jan 22, 2015
@krig
Copy link
Contributor Author

krig commented Jan 22, 2015

Updating with this regex fixes the history test case, not only that, it manages to discover an additional transition thanks to this change :) So this seems good now.

krig added a commit to krig/crmsh that referenced this pull request Jan 23, 2015
With new transition regex, the partial transition number 47
is also detected and available.
krig added a commit to krig/crmsh that referenced this pull request Jan 26, 2015
krig added a commit to krig/crmsh that referenced this pull request Jan 26, 2015
With new transition regex, the partial transition number 47
is also detected and available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants