Skip to content

Cleaned up Go code directory structure #99

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 8, 2024
Merged

Cleaned up Go code directory structure #99

merged 1 commit into from
Oct 8, 2024

Conversation

p-t-u
Copy link
Contributor

@p-t-u p-t-u commented Oct 2, 2024 •
edited
Loading

  • build-ocb now also works under Linux
  • Moved cmd/collector to observecol
  • Renamed/moved directory cmd to internal
  • Canonicalized the naming of our Go modules

Description

OB-37278: Clean up Go code directory structure

@p-t-u p-t-u marked this pull request as ready for review October 3, 2024 16:39
@obs-gh-alexlew
Copy link
Collaborator

no goreleaser changes required? changing the package name didn't affect the executable name?

obs-gh-alexlew
obs-gh-alexlew reviewed Oct 3, 2024
View reviewed changes
obs-gh-alexlew
obs-gh-alexlew reviewed Oct 3, 2024
View reviewed changes
obs-gh-alexlew
obs-gh-alexlew reviewed Oct 3, 2024
View reviewed changes
obs-gh-alexlew
obs-gh-alexlew reviewed Oct 3, 2024
View reviewed changes
@p-t-u
Copy link
Contributor Author

p-t-u commented Oct 3, 2024

no goreleaser changes required? changing the package name didn't affect the executable name?

I don't think so. goreleaser is still happy. Here's some relevant output from the snapshot build:

    • added new artifact                             name=observe-agent type=Binary path=dist/default_linux_386/observe-agent                                                                            
    • added new artifact                             name=observe-agent type=Binary path=dist/default_linux_arm64/observe-agent    
    • added new artifact                             name=observe-agent type=Binary path=dist/default_darwin_amd64_v1/observe-agent                                                                      
    • added new artifact                             name=observe-agent.exe type=Binary path=dist/windows_build_windows_386/observe-agent.exe                                                            
    • added new artifact                             name=observe-agent type=Binary path=dist/default_linux_amd64_v1/observe-agent                                                                       
    • added new artifact                             name=observe-agent.exe type=Binary path=dist/windows_build_windows_amd64_v1/observe-agent.exe                                                       
    • added new artifact                             name=observe-agent type=Binary path=dist/default_darwin_arm64/observe-agent

The final segment of the package name is also still "observe-agent", so no change needed there. And the "github.com..." stuff is of course never part of the executable name.

goreleaser eventually fails with these kind of errors:

#11 [directories 2/2] RUN mkdir -p /var/lib/observe-agent/filestorage                                                                                                                                    
#11 0.490 exec /bin/sh: exec format error                                                                                                                                                                
#11 ERROR: process "/bin/sh -c mkdir -p /var/lib/observe-agent/filestorage" did not complete successfully: exit code: 1

but I read that's normal for local builds outside of GitHub CI, and it happens after the executables are built successfully.

obs-gh-alexlew
obs-gh-alexlew reviewed Oct 4, 2024
View reviewed changes
internal/commands/version/version.go Outdated
@@ -19,12 +19,12 @@ var versionCmd = &cobra.Command{
is based on the package release.`,
Run: func(cmd *cobra.Command, args []string) {
version := getVersion()
fmt.Printf("observe-agent version: %s\n", version)
fmt.Printf("github.com/observeinc/observe-agent version: %s\n", version)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Think this also doesn't need the prefix

@p-t-u
Refactored module names and directory layout
0f5d87a 
Moved cmd/collector to observecol, because the cmd subdirectory is
reserved for packages that are executables, while our observe-agent
executable is actually defined by the root directory of the repo.

Renamed/moved directory cmd to internal. All of the packages under the
former cmd directory are really internal packages, not executables.
Changed the module name of the observe-agent module from "observe-agent"
to "github.com/observeinc/observe-agent".

Changed the module name of the observecol module (the one generated by
ocb) from "observecol" to
"github.com/observeinc/observe-agent/observecol".

Ran `go mod tidy` in all our modules (including
observek8sattributesprocessor) and sorted out all the replacement and
versioning issues. Everything is squeaky clean now.

Updated a bunch of dependencies as a byproduct of `go mod tidy`.

build-ocb now also works under Linux (after setting the OCB variable in
the Makefile correctly).
@p-t-u
Copy link
Contributor Author

p-t-u commented Oct 7, 2024

Regarding go.sum: I removed the go.sum files in this commit because I was going crazy trying to resolve the merge conflicts over and over again. Most people on the interwebs recommend checking the file in, even if it tends to create a lot of merge conflicts, because other people can use it to verify that their local dependencies really match what the author of the library used. I'm happy to bring back the go.sum files, but I recommend doing a separate PR to keep this one lean.

@p-t-u p-t-u merged commit e91cdb1 into main Oct 8, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@obs-gh-alexlew obs-gh-alexlew obs-gh-alexlew approved these changes

Assignees

@obs-gh-alexlew obs-gh-alexlew

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@p-t-u @obs-gh-alexlew