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

Bumping up mesos-go, thrift, and gorealis dependencies. #120

Merged
merged 1 commit into from
Oct 29, 2021

Conversation

ridv
Copy link
Contributor

@ridv ridv commented Oct 28, 2021

Mesos-go v0.0.3-snapshot -> v0.0.11
gorealis v1.1.0 -> v1.23.0
thrift v0.0.0-snapshot -> v0.14.0

@ridv ridv merged commit 2b5c3a4 into paypal:develop Oct 29, 2021
@ridv ridv deleted the mesos-go-upgrade branch October 29, 2021 02:12
Copy link

@lenhattan86 lenhattan86 left a comment

Choose a reason for hiding this comment

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

overall looks good to me.

github.com/mitchellh/mapstructure v1.3.2 // indirect
github.com/niemeyer/pretty v0.0.0-20200227124842-a10e7caefd8e // indirect
github.com/paypal/gorealis v1.1.0
github.com/paypal/gorealis v1.23.0

Choose a reason for hiding this comment

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

can I know why we have to stick with gorealis v1? I don't remember it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gorealis v2 changed some of the underlying objects around. We can probably help the DCE-go maintainers to move over to the newest version though, shouldn't be too hard.

@@ -26,8 +26,8 @@ import (
"syscall"
"time"

exec "github.com/mesos/mesos-go/executor"
mesos "github.com/mesos/mesos-go/mesosproto"
exec "github.com/mesos/mesos-go/api/v0/executor"

Choose a reason for hiding this comment

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

I saw some use exec, some don't.
also we have os/exec. I think it may confuse us. It would be best to make it consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is something definitely worth looking into, for this change I just wanted to make the changes as minimal as possible.

@@ -20,7 +20,7 @@ import (
"strconv"
"testing"

"github.com/mesos/mesos-go/mesosproto"
"github.com/mesos/mesos-go/api/v0/mesosproto"

Choose a reason for hiding this comment

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

in the impl.go, we use "mesos", in this file we don't.

Choose a reason for hiding this comment

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

some other files don't use mesos "..." too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, there's very little consistency here in terms of renaming imports.

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.

3 participants