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

ship prebuilt gometro on RHEL #202

Merged
merged 8 commits into from
Aug 20, 2018
Merged

ship prebuilt gometro on RHEL #202

merged 8 commits into from
Aug 20, 2018

Conversation

arbll
Copy link
Member

@arbll arbll commented Jul 31, 2018

Our builder is based on Centos5. Building go-metro with a recent version of go on it was deemed too much work (and unsupported). Instead we'll ship a prebuilt binary.
It was built in almost the same builder docker image with centos6 instead of centos5.

@@ -1,5 +1,5 @@
name "datadog-gohai"
default_version "last-stable"
default_version "arbll/go1.10"
Copy link
Member Author

Choose a reason for hiding this comment

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

Will remove this after DataDog/gohai#59 is merged

Copy link
Member

@truthbk truthbk left a comment

Choose a reason for hiding this comment

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

This looks good, but there's an underlying problem. I saw your update to the RPM builder bumped go there to 1.10.3 but the docs clearly state CentOS/RHEL 5 is not supported. I don't think I'm cool with building on an unsupported platform.

I would rather have us build the pure-go (I don't think gohai has any cgo, does it?) gohai on 1.10 on a supported platform and download the binary here like you're doing for go-metro. I'm happy to discuss.

command "mkdir -p /var/cache/omnibus/src/datadog-metro/src/github.com/DataDog", :env => env
command "#{gobin} get -v -d github.com/DataDog/go-metro", :env => env, :cwd => "/var/cache/omnibus/src/datadog-metro"
command "git checkout #{default_version} && git pull", :env => env, :cwd => "/var/cache/omnibus/src/datadog-metro/src/github.com/DataDog/go-metro"
command "#{gobin} get -v -d github.com/cihub/seelog", :env => env, :cwd => "/var/cache/omnibus/src/datadog-metro"
Copy link
Member

Choose a reason for hiding this comment

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

Going to create a low-priority card so we vendor deps for go-metro.

Copy link
Member

Choose a reason for hiding this comment

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

Also, I believe the plan is to say go-metro is only supported on CentOS 6+ (for RHEL/CentOS customers), right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed

Copy link
Member

@truthbk truthbk left a comment

Choose a reason for hiding this comment

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

Just realized this...

@@ -10,6 +10,11 @@

dependency "libpcap"

if ohai["platform_family"] == "rhel"
source :url => "https://s3.amazonaws.com/dd-agent/go-metro/gometro-centos6",
Copy link
Member

Choose a reason for hiding this comment

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

Can you version the gometro-centos6 binary? For instance gometro-centos6-1.0.0 or whatever the current version is? Not sure the project is even properly versioned. There's actually a big PR I'd like to merge, maybe that could be 1.0.0. Anyways, we probably want to do something like that.

@arbll
Copy link
Member Author

arbll commented Aug 20, 2018

Added a version number (and tagged gometro https://github.com/DataDog/go-metro/tags)

Copy link
Member

@truthbk truthbk left a comment

Choose a reason for hiding this comment

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

Added a couple more notes. Thanks for versioning go-metro

@@ -10,6 +10,11 @@

dependency "libpcap"

if ohai["platform_family"] == "rhel"
source :url => "https://s3.amazonaws.com/dd-agent/go-metro/gometro-centos6-1.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

You probably want to do this here:

version "1.0.0" do
  source :sha256 => "a6fb05dcbe0f412eaac44095db67a8d71cce6c66dc900b0b78258de4ee43bf2f"
end

if ohai["platform_family"] == "rhel"
  source :url => "https://s3.amazonaws.com/dd-agent/go-metro/gometro-centos6-#{version}",
end

command "mv gometro-centos6 #{install_dir}/bin/go-metro"
else
command "mkdir -p /var/cache/omnibus/src/datadog-metro/src/github.com/DataDog", :env => env
command "#{gobin} get -v -d github.com/DataDog/go-metro", :env => env, :cwd => "/var/cache/omnibus/src/datadog-metro"
Copy link
Member

Choose a reason for hiding this comment

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

We should probably tag the repo (which it's not), I think it's just tagged with last-stable, and make sure we check out the right code. This wasn't great, you can do it yourself, or you can create a card for it.

Copy link
Member

@truthbk truthbk left a comment

Choose a reason for hiding this comment

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

I think this looks good now! 🙇

@arbll arbll merged commit a8e413a into master Aug 20, 2018
@arbll arbll deleted the arbll/ship-prebuilt-gometro branch August 20, 2018 15:09
@arbll arbll mentioned this pull request Aug 21, 2018
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.

2 participants