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

Refactor and clean up code base #46

Merged
merged 68 commits into from
Aug 14, 2024
Merged

Refactor and clean up code base #46

merged 68 commits into from
Aug 14, 2024

Conversation

davidallendj
Copy link
Collaborator

@davidallendj davidallendj commented Jul 23, 2024

Recreated PR from #43.

This PR covers a range of changes to be made to Magellan's internal API and command-line interface, including but not limited to:

  • Changing the CLI flag names to be more consistent
  • Changing the logger to JSON format and write all errors to stderr to be consumed
  • Changing the internal API signatures of some functions to be more descriptive in other areas (like in tests)
  • Changing how files are saved to disk to use the "hive" partitioning format
  • Reimplementing some of the CollectAll function to use the CrawlBMC
  • Condensing the output to only include desired information (solved by using CrawlBMC)
  • Removing old code using bmclib and that are unused and bmclib dependency
  • Removing commented out code that's not being used
  • Improving the scan subcommand input parameters

The list above is subject to change as needed and aims to not introduce any new features into the current code base. It will likely break API compatibility in some areas to some extent as things are being changed, cleaned up, and moved around. Merging this PR will also require updating the parsing code to handle the output received from magellan in SMD.

Additionally, this PR addresses issues #8, #18, and #15.

@davidallendj davidallendj reopened this Jul 24, 2024
@synackd
Copy link
Contributor

synackd commented Aug 13, 2024

The Makefile GOPATH stuff looks good.

@davidallendj
Copy link
Collaborator Author

The lint errors should be fixed now.

@synackd
Copy link
Contributor

synackd commented Aug 13, 2024

They indeed are!

However, the unit tests are failing:

Executing target: test
go test -race -covermode=atomic -coverprofile=coverage.out -coverpkg=./... ./...
?       github.com/OpenCHAMI/magellan   [no test files]
?       github.com/OpenCHAMI/magellan/cmd       [no test files]
?       github.com/OpenCHAMI/magellan/internal  [no test files]
?       github.com/OpenCHAMI/magellan/internal/cache    [no test files]
?       github.com/OpenCHAMI/magellan/internal/cache/postgresql [no test files]
?       github.com/OpenCHAMI/magellan/pkg/client        [no test files]
?       github.com/OpenCHAMI/magellan/pkg/crawler       [no test files]
?       github.com/OpenCHAMI/magellan/internal/cache/sqlite     [no test files]
?       github.com/OpenCHAMI/magellan/internal/util     [no test files]
?       github.com/OpenCHAMI/magellan/pkg/auth  [no test files]
--- FAIL: TestScanAndCollect (0.00s)
    api_test.go:38: expected to find at least one BMC node, but found none
--- FAIL: TestRedfishV1Availability (0.00s)
    compatibility_test.go:36: failed to make request to BMC: failed to make request: Get "localhost/redfish/v1": unsupported protocol scheme ""
{"level":"error","error":"failed to make request: Get \"localhost/redfish/v1\": unsupported protocol scheme \"\"","time":"2024-08-13T18:06:00Z","message":"failed to make request"}
{"level":"error","error":"endpoint must starts with http or https","time":"2024-08-13T18:06:00Z","message":"failed to connect to BMC"}
--- FAIL: TestExpectedProperties (0.00s)
    compatibility_test.go:91: failed to crawl BMC: endpoint must starts with http or https
FAIL
coverage: 15.8% of statements in ./...
FAIL    github.com/OpenCHAMI/magellan/tests     0.023s
FAIL

@davidallendj
Copy link
Collaborator Author

davidallendj commented Aug 13, 2024

They indeed are!

However, the unit tests are failing:

Executing target: test
go test -race -covermode=atomic -coverprofile=coverage.out -coverpkg=./... ./...
?       github.com/OpenCHAMI/magellan   [no test files]
?       github.com/OpenCHAMI/magellan/cmd       [no test files]
?       github.com/OpenCHAMI/magellan/internal  [no test files]
?       github.com/OpenCHAMI/magellan/internal/cache    [no test files]
?       github.com/OpenCHAMI/magellan/internal/cache/postgresql [no test files]
?       github.com/OpenCHAMI/magellan/pkg/client        [no test files]
?       github.com/OpenCHAMI/magellan/pkg/crawler       [no test files]
?       github.com/OpenCHAMI/magellan/internal/cache/sqlite     [no test files]
?       github.com/OpenCHAMI/magellan/internal/util     [no test files]
?       github.com/OpenCHAMI/magellan/pkg/auth  [no test files]
--- FAIL: TestScanAndCollect (0.00s)
    api_test.go:38: expected to find at least one BMC node, but found none
--- FAIL: TestRedfishV1Availability (0.00s)
    compatibility_test.go:36: failed to make request to BMC: failed to make request: Get "localhost/redfish/v1": unsupported protocol scheme ""
{"level":"error","error":"failed to make request: Get \"localhost/redfish/v1\": unsupported protocol scheme \"\"","time":"2024-08-13T18:06:00Z","message":"failed to make request"}
{"level":"error","error":"endpoint must starts with http or https","time":"2024-08-13T18:06:00Z","message":"failed to connect to BMC"}
--- FAIL: TestExpectedProperties (0.00s)
    compatibility_test.go:91: failed to crawl BMC: endpoint must starts with http or https
FAIL
coverage: 15.8% of statements in ./...
FAIL    github.com/OpenCHAMI/magellan/tests     0.023s
FAIL

The tests are being worked on in another PR #42. This PR is a prerequisite to finish that one since there are API changes.

@synackd
Copy link
Contributor

synackd commented Aug 13, 2024

Understood. In that case, the only thing left from my perspective is deciding whether to keep the flag format changes (- versus .) or not.

Copy link
Contributor

@synackd synackd left a comment

Choose a reason for hiding this comment

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

I made an updated review off of the latest changes for the discussion on flag format (. versus -). I also found some errors occurring with flags from the collect subcommand, so I'm requesting changes to fix those.

cmd/update.go Outdated Show resolved Hide resolved
cmd/update.go Outdated Show resolved Hide resolved
cmd/collect.go Outdated Show resolved Hide resolved
cmd/update.go Outdated Show resolved Hide resolved
Copy link
Contributor

@synackd synackd left a comment

Choose a reason for hiding this comment

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

Getting:

{"level":"error","error":"failed to make request: Post \"https:/foobar.openchami.cluster/hsm/v2/Inventory/RedfishEndpoints\": http: no Host in request URL","time":"2024-08-14T16:14:41Z","message":"failed to add Redfish endpoint"}

because of replacing // with /.

cmd/collect.go Outdated Show resolved Hide resolved
@davidallendj davidallendj linked an issue Aug 14, 2024 that may be closed by this pull request
Copy link
Contributor

@synackd synackd left a comment

Choose a reason for hiding this comment

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

I found a small issue that is causing a 503 error during testing that is an easy fix. Upon testing with this fix, I got corrrect 401 and 409 responses when running collect on a populated SMD, but got a 500 when populating a fresh SMD. I'm not currently sure if this is with SMD or Magellan (I'm leaning towards the former), we'll have to dig deeper to find out.

internal/url/url.go Outdated Show resolved Hide resolved
@synackd
Copy link
Contributor

synackd commented Aug 14, 2024

LGTM

@davidallendj davidallendj merged commit b8b4aed into main Aug 14, 2024
@davidallendj davidallendj deleted the refactor branch August 14, 2024 20:46
@davidallendj davidallendj removed needs review Issues or pull requests that needs to be reviewed needs testing labels Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Code refactoring and/or renaming
Projects
None yet
3 participants