feat: swift export by notque · Pull Request #178 · sapcc/hermescli

notque · 2025-01-14T20:24:10Z

add swift export to hermescli to enable fast and easy swift exports of audit data.

coveralls · 2025-01-14T20:26:22Z

Pull Request Test Coverage Report for Build 12939904652

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

29 of 281 (10.32%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-1.1%) to 12.733%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
client/print.go	0	20	0.0%
client/swift.go	0	57	0.0%
client/export.go	29	204	14.22%

Totals
Change from base Build 12751712796:	-1.1%
Covered Lines:	116
Relevant Lines:	911

💛 - Coveralls

client/export.go

client/swift.go

client/export.go

client/swift.go

std function slices.Contains Co-authored-by: Stefan Majewsky <majewsky@gmx.net>

Co-authored-by: Stefan Majewsky <majewsky@gmx.net>

…ls, as opposed to the export

This reverts commit d3922da.

This reverts commit 5941370. I wasn't thinking clearly here, this moves it outside of where it should be. Logically, I see what I was going for, but it doesn't make any sense

notque · 2025-01-23T21:09:19Z

@majewsky is this sane? Have I addressed your feedback appropriately?

kayrus

Thanks a lot for this PR! I wanted to implement this logic since I developed this tool, but I had other priorities.
General note is about exported functions/methods/types. Do they really need to be exported? Is there a need to consume them outside of this project? For example the ProgressReader struct or ParseExportFormat func

Another type of notes, I see a lot of go deps, which implement the Write/Read methods. Wouldn't it be more efficient to transfer the data from the hermes service output directly to the swift using pipes instead of collecting all this data in memory and only then upload to swift?

My gut feeling tells me that there are further minor issues, but this is a very good start anyway.

@stefanhipfel thank you for your review as well!

client/export.go

notque · 2025-01-23T22:34:54Z

Wouldn't it be more efficient to transfer the data from the hermes service output directly to the swift using pipes instead of collecting all this data in memory and only then upload to swift?

The reality is this is the, good for now solution. This whole thing will ultimately be replaced by Hermes doing this in the future, but there are some hold ups on that happening at the moment.

I am trying to urgently get a solution out, I have meetings where I need to provide this next week.

I understand that's not the best response to feedback on a PR. I can try to do more, but I really would like to get a good enough solution out, that I can get released and available to customers asap.

I apologize for the attempt to get this through, however I'm trying to turn a couple rather contentious meetings next week, into reasonably lovely meetings.

client/export.go

kayrus · 2025-01-23T22:29:27Z

client/export.go

+		// Create upload progress bar
+		uploadBar := pb.Full.Start64(int64(buf.Len()))
+		uploadBar.Set(pb.Bytes, true)
+		uploadBar.SetWidth(80)


is it a static width? I just grepped a project and found this:

hermescli$ grep -r pb.SetWidth vendor/github.com/cheggaaa/pb/v3/pb.go: pb.SetWidth(s.AdaptiveElWidth())

maybe it make sense to do this dynamic or configurable

That's fair.

Thinking about it, I'm not sure making this configurable adds anything of value. It becomes another option in the list that is fairly meaningless when it's just a nice to have indication of the process.

I've committed a change which makes the terminal width automatic. Does that satisfy your request?

client/export.go

notque · 2025-01-23T23:23:16Z

see a lot of go deps, which implement the Write/Read methods. Wouldn't it be more efficient to transfer the data from the hermes service output directly to the swift using pipes instead of collecting all this data in memory and only then upload to swift?

I thought this was a feature to not allow people to export the entire system. :)

It would be more efficient. It would also be more complex, and wouldn't show the total progress as far as I can tell. I don't know, I feel like this is fairly functional like this but I recognize and understand what you're asking for here.

notque · 2025-01-27T04:20:58Z

see a lot of go deps, which implement the Write/Read methods. Wouldn't it be more efficient to transfer the data from the hermes service output directly to the swift using pipes instead of collecting all this data in memory and only then upload to swift?

I thought this was a feature to not allow people to export the entire system. :)

It would be more efficient. It would also be more complex, and wouldn't show the total progress as far as I can tell. I don't know, I feel like this is fairly functional like this but I recognize and understand what you're asking for here.

For giggles I tried to build this out, and it was a nightmare I didn't get fully working. Yaml, Json, and CSV all have different requirements, and trying to stream events like that just gets ridiculously complex.

If you want to explain out what you're thinking of here, maybe I'm totally wrong in how I'm doing it. Like, moving to json lines or something would make it more achievable.

kayrus · 2025-01-27T09:28:32Z

I thought this was a feature to not allow people to export the entire system. :)

if this is PR has a time pressure, then leave it as is. We can improve the performance later on demand.

feat: swift export

e1f4e7a

notque requested a review from majewsky January 14, 2025 20:24

notque self-assigned this Jan 14, 2025

notque requested a review from SuperSandro2000 January 15, 2025 14:42

SuperSandro2000 reviewed Jan 15, 2025

View reviewed changes

client/export.go Show resolved Hide resolved

client/export.go Show resolved Hide resolved

client/swift.go Outdated Show resolved Hide resolved

majewsky reviewed Jan 15, 2025

View reviewed changes

notque and others added 12 commits January 15, 2025 09:46

Update client/export.go

de87673

std function slices.Contains Co-authored-by: Stefan Majewsky <majewsky@gmx.net>

import slices

8439e5f

use headers.ToOpts shorthand

2ba7b01

Co-authored-by: Stefan Majewsky <majewsky@gmx.net>

cleaner implementation of container EnsureExists

06305be

Co-authored-by: Stefan Majewsky <majewsky@gmx.net>

remove import http, logg, add ctx

56c9691

change struct to make more sense around a single idea, the file detai…

4509fb2

…ls, as opposed to the export

convertToRequestedFormat func to simplify RunE

6be8dd5

progress bar should be in swift.go now i think

5941370

move ProgressReader struct to top

d3922da

Revert "move ProgressReader struct to top"

e9f85e5

This reverts commit d3922da.

Revert "progress bar should be in swift.go now i think"

6593ce4

This reverts commit 5941370. I wasn't thinking clearly here, this moves it outside of where it should be. Logically, I see what I was going for, but it doesn't make any sense

ExportFormat

a9bb334

kayrus reviewed Jan 23, 2025

View reviewed changes

client/export.go Outdated Show resolved Hide resolved

notque added 2 commits January 23, 2025 15:39

Multiline allExportFormats for readability

c1484f6

progressReader and parseExportFormat not exported

eeec680

kayrus reviewed Jan 23, 2025

View reviewed changes

notque added 5 commits January 23, 2025 15:48

fix err where err was already defined above

07438c8

const for timestamp format with comment as to why

4869805

remove debug logging that provides no logging

fdcdd42

automatic terminal width for progress bar

dc47073

remove unneeded default specification of writer for pb

cf3ad8c

kayrus approved these changes Jan 27, 2025

View reviewed changes

notque merged commit 098e825 into master Jan 27, 2025

notque deleted the export_swift branch January 27, 2025 17:09

Conversation

notque commented Jan 14, 2025

Uh oh!

coveralls commented Jan 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 12939904652

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

notque commented Jan 23, 2025

Uh oh!

kayrus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

notque commented Jan 23, 2025

Uh oh!

Uh oh!

Uh oh!

kayrus Jan 23, 2025

Choose a reason for hiding this comment

Uh oh!

notque Jan 23, 2025

Choose a reason for hiding this comment

Uh oh!

notque Jan 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

notque commented Jan 23, 2025

Uh oh!

notque commented Jan 27, 2025

Uh oh!

kayrus commented Jan 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

coveralls commented Jan 14, 2025 •

edited

Loading