Skip to content

Conversation

@efd6
Copy link
Contributor

@efd6 efd6 commented Jan 16, 2022

This makes to output of elastic-package version informative when the go.mod-based install within integrations is used. It does not prevent the "CommitHash is undefined" warning since that is based on build-time var substitution. The logic required to properly handle update detection is reasonably involved, so I have not added this (see here for the general case — not all of that would be required here).

The output of elastic-package version when it has been installed using go install github.com/elastic/elastic-package@latest or by go install github.com/elastic/elastic-package from within a module with it written as a require would look something like this

elastic-package v0.43.1 version-hash 032f343 (build time: 2052-01-18T19:38:06+4:30)

Please take a look.

@efd6 efd6 requested a review from mtojek January 16, 2022 23:20
@elasticmachine
Copy link
Collaborator

elasticmachine commented Jan 16, 2022

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-01-18T09:02:55.923+0000

  • Duration: 27 min 57 sec

  • Commit: 08f3093

Test stats 🧪

Test Results
Failed 0
Passed 459
Skipped 0
Total 459

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

@efd6
Copy link
Contributor Author

efd6 commented Jan 17, 2022

Failure looks to be unrelated.

@efd6
Copy link
Contributor Author

efd6 commented Jan 17, 2022

/test

@jsoriano jsoriano added the Team:Ecosystem Label for the Packages Ecosystem team label Jan 17, 2022
@efd6
Copy link
Contributor Author

efd6 commented Jan 17, 2022

/test

Copy link
Contributor

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

Hey @efd6! thanks for the PR. Regarding failing tests, please merge the latest mainline as we pushed a fix there.

Could you please add to the PR description the sample output of the version command?

@mtojek mtojek requested a review from a team January 18, 2022 08:44
…l github.com/elastic/elastic-package

This makes to output of elastic-package version informative when the go.mod-based
install within integrations is used.
@efd6
Copy link
Contributor Author

efd6 commented Jan 18, 2022

The output of the version command will remain the same as previously when the Tag is set using -ldflags=-X. I can't demonstrate until there is a merged commit with this code in, but I will construct a hand drawn facsimile.

Copy link
Contributor

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

I'm a bit concerned about the profiles logic, but in the worst case, we can revert this change.

If the CI is happy, I'm fine too.

@mtojek mtojek merged commit 65f8e5e into elastic:master Jan 18, 2022
@efd6
Copy link
Contributor Author

efd6 commented Jan 18, 2022

@mtojek With this change in and installing this particular commit I get this:

$ go install github.com/elastic/elastic-package@65f8e5
go: downloading github.com/elastic/elastic-package v0.33.2-0.20220118093250-65f8e5e840b5
$ elastic-package version
elastic-package has been installed.
elastic-package v0.33.2-0.20220118093250-65f8e5e840b5 version-hash undefined (build time: unknown)

If it's installed with a tagged version the pseudoversion suffix will be absent.

What are your concerns about the profiles logic?

@efd6 efd6 deleted the modver branch January 18, 2022 10:19
@mtojek
Copy link
Contributor

mtojek commented Jan 18, 2022

@efd6 I'm curious what would be the content of the profile.json file if you create a new profile:

cat ~/.elastic-package/profiles/default/profile.json

In my case it's now:

{
  "name": "default",
  "date_created": "2022-01-11T16:50:19.230744+01:00",
  "user": "marcin.tojek",
  "version": "30faef5",
  "path": "/Users/marcin.tojek/.elastic-package/profiles/default"
}

version contains a hash.

elastic-package v0.33.2-0.20220118093250-65f8e5e840b5 version-hash undefined (build time: unknown)

It still looks strange, so maybe we should fully switch to the debug.ReadBuildInfo? We just need to make sure that the logic uses it correctly to check the ~/.elastic-package dir in terms of stack defs and profiles. WDYT?

@efd6
Copy link
Contributor Author

efd6 commented Jan 18, 2022

In the case with the install I just did, I see

$ cat ~/.elastic-package/profiles/default/profile.json
{
  "name": "default",
  "date_created": "2022-01-18T20:46:06.107629+10:30",
  "user": "xxx",
  "version": "undefined",
  "path": "/Users/xxx/.elastic-package/profiles/default"
}

Using only the embedded version information should be fine, in fact the hash gives no ordering to versions as is noted in output when there is no version in the current code. The work that's needed is much less than is done in ugbt. If you'd like to go that route I'm happy to discuss.

@mtojek
Copy link
Contributor

mtojek commented Jan 18, 2022

If there is a way to modify the code to properly return version for both make build and go get and go get @master, then I'm more than happy to look into a draft PR :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Team:Ecosystem Label for the Packages Ecosystem team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants