Can not unmarshal the attribute [CurrentStep] in Event [Deployment Info] to object EventDeploymentInfo about go-marathon HOT 10 CLOSED

Do you use of version of go-marathon recent enough to include #158? There was a fix for a breaking API change in Marathon 1.x in the area you are referring to.

If you do: Can you tell us what version of Marathon you are working with?

Thanks.

from go-marathon.

I am using marathon v1.1.1 and go-marathon on branch master.
I think the PR #158 has broken the EventDeploymentInfo's unmarshalling

EventDeploymentInfo.CurrentStep is still in V1 form. We should change it back by using DeploymentStepV1

from go-marathon.

/cc @kevinschoon who worked on the PR back then.

from go-marathon.

Hi --

Unfortunately #158 only fixed unmarshaling from the /v2/deployments endpoint and not the event stream. We would likely need to add some conditional logic here to support both older and newer versions of Marathon.

If you can provide steps to replicate the problem I can try to apply a fix. I tried locally but couldn't actually get it to generate an error against a local v1.1.1 Marathon instance.

from go-marathon.

First of all, I use marathonClient.AddEventListener() with Config.EventsTransport set EventsTransportSSE which is using marathon api /v2/events. And the event json could be logged here by printing content. When scaling an app, a event with id EventIDDeploymentInfo would be catched there. Then it could be found that the json content dose not match the event.Event chosen by GetEvent as I mentioned before. As a result, the EventDeploymentInfo.CurrentStep decodes nothing. It can be reproduced by checking it.

Actually, the json element currentStep has been keeping the same form as DeploymentStepV1. I think it is a mistake from #158 . So I fix this by using DeploymentStepV1 in PR #176 .

from go-marathon.

@wu8685 I believe @kevinschoon is right: #158 apparently did not change the definition of DeploymentStep. That PR just introduced a new version of it as DeploymentStepV1 for the deployments resource which happened to be in use at some other place (the events resource) already. I checked out a copy of go-marathon prior to #158 and verified that EventDeploymentInfo.CurrentStep could not be parsed with Marathon 0.15.2.

I could also verify that at least Marathon 0.15.2 was already using the new format. I'm going to check 0.14 too later, although I'm not sure if it's worth still providing backwards compatibility for that branch. If we deem it's not, then we can just move forward with @wu8685's PR without worrying about adding another conditional switch.

from go-marathon.

I went back as far as Marathon 0.10 but couldn't find a version parseable with the old (current) step definition. I'm either doing something wrong or the step definition was never working.

from go-marathon.

@timoreimann I m afraid that I'm getting a little confused. Which definition in the following is the EventDeploymentInfo.CurrentStep and old (current) step definition you mentioned in your last two comments?

type DeploymentStep struct {
    Action string `json:"action"`
    App    string `json:"app"`
}

type DeploymentStepV1 struct {
    Actions []struct {
        Type string `json:"type"`
        App  string `json:"app"`
    }
}

Do you mean that the older Marathon responses different json schema about currentStep from v1.1.1?

from go-marathon.

Sorry, I should have been a bit clearer.

All Marathon versions I have tested (up to and including 0.10) seem to be using a currentStep definition equal to DeploymentStepV1, i.e.,

type DeploymentStepV1 struct {
    Actions []struct {
        Type string `json:"type"`
        App  string `json:"app"`
    }
}

The schema is not different from what we observe in Marathon 1.1.1, which led me to believe that our present definition of

type DeploymentStep struct {
    Action string `json:"action"`
    App    string `json:"app"`
}

has either never worked or only worked for rather old Marathon versions at least older than 0.10. To me, this means we can move forward with your PR without having to worry about any backwards compatibility concerns.

Does this help?

from go-marathon.

@timoreimann Yes. Thanks for your explanation.

from go-marathon.