Skip to content
This repository has been archived by the owner on Feb 9, 2024. It is now read-only.

ignore overall_state_modified #13

Closed
wants to merge 1 commit into from
Closed

Conversation

dd-caleb
Copy link

@dd-caleb dd-caleb commented Mar 2, 2017

Hi,

We added an additional field to keep track of the last time the overall state was modified. It appears this broke this tool.

Apologies for introducing a backwards-incompatible change. We were working under the assumption that whereas removing or renaming fields would cause problems for customers, simply adding a new field would not, but it looks like that assumption was wrong.

I presume this should fix the problem, though it's probably worth updating the code in some way to be more tolerant of added fields.

@gyoza
Copy link

gyoza commented Mar 2, 2017

I've added this to my personal DogPush yesterday in attempts to get this to work, even after adding this field into the python script I am still unable to change our monitors. It just seems like the API is not accepting any data.

#12

@dd-caleb
Copy link
Author

dd-caleb commented Mar 2, 2017

OK, I will spend some time today attempting to debug this library. We were not seeing issues updating monitors via the API, but perhaps there's a code-path this library goes through that we didn't test.

@gyoza
Copy link

gyoza commented Mar 2, 2017

This is what I did. I added the ignore to Overall_state_modified, (I actually have that field in my yaml files. Should we take it out? ) and changed locked; true on a few of my monitors to locked: false and tried to push it out and no luck.

@dd-caleb
Copy link
Author

dd-caleb commented Mar 2, 2017

So I'm testing this locally, updated a message field on one of the monitors, and was able to push it. Perhaps there's something special about the monitors you're trying to update?

Would it be possible to share them with me so I can try and reproduce the issue?

Also, though this is probably unrelated, I noticed the initial monitor creation had:

escalation_message: !!python/unicode '', 

Which made the YAML parser fail... changing it to just an empty string made it work again.

Thanks for taking the time to build this library and sorry for all the trouble.

@gyoza
Copy link

gyoza commented Mar 2, 2017

Figured it out, need to remove all the latent overall_state_modified from our yamls.

@dd-caleb
Copy link
Author

dd-caleb commented Mar 2, 2017

OK, great. I'll close this PR since you already fixed this in another one.

Ping me if you have any other issues: [email protected]. I'll make a note that we need to be more cautious about adding new fields in the future.

@dd-caleb dd-caleb closed this Mar 2, 2017
@dd-caleb dd-caleb deleted the patch-1 branch March 2, 2017 18:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants