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.

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

Derive the swagger version from the go.mod file #1709

Merged
merged 6 commits into from
Nov 23, 2020
Merged

Derive the swagger version from the go.mod file #1709

merged 6 commits into from
Nov 23, 2020

Conversation

tsachiherman
Copy link
Contributor

@tsachiherman tsachiherman commented Nov 17, 2020 •
edited
Loading

Summary

We have the swagger version hard coded in the configure_dev-deps.sh file, whereas all the other go modules have their version stored in the go.mod file.

This change takes the module version from the go.mod file for the swagger as well.

This is a followup on #1704

Test Plan

Since tip-of-tree on the swagger repo would break our build, a successful build ensures that the solution works as expected.

@tsachiherman tsachiherman self-assigned this Nov 17, 2020
@tsachiherman tsachiherman marked this pull request as ready for review November 17, 2020 19:55
egieseke
egieseke reviewed Nov 17, 2020
View reviewed changes
Copy link
Contributor

@egieseke egieseke left a comment

Choose a reason for hiding this comment

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

Are the telemetry changes intentional?

go.mod
@@ -17,6 +17,7 @@ require (
github.com/gen2brain/beeep v0.0.0-20180718162406-4e430518395f
github.com/getkin/kin-openapi v0.22.0
github.com/go-chi/chi v4.1.2+incompatible // indirect
github.com/go-swagger/go-swagger v0.25.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want this dependency in go.mod since it is used by our build process but not depended on by our software?

Copy link
Contributor Author

@tsachiherman tsachiherman Nov 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It's currently required by our makefile.. so I would lean toward "yes".
But that's not the only way to achieve that :
we could have another go.mod/go.sum in a separate directory, and fetch the go-swagger from there.

I think that it makes it simpler to know that all the dependencies are in one location, though.

logging/telemetryCommon.go
ReportHistoryLevel logrus.Level `json:"-"` // text marshalers which breaks our backward compatibility.
FilePath string // Path to file on disk, if any
ChainID string `json:"-"`
SessionGUID string `json:"-"`
UserName string
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these telemetry config changes appropriate for this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The downside of having a single go.mod for both the go-swagger and our code is that we need to synchronize versions of shared components. These changes are backward compatible, so I could do these without applying this PR, and have the PR applied afterward. ( but I don't think it would add much more meaningful granularity.. )

If you think that these changes need to be splitted off, please let me know and I'll create a separate PR for these.

@onetechnical onetechnical requested a review from btoll November 23, 2020 16:53
@btoll
Copy link

btoll commented Nov 23, 2020

@tsachiherman The changes make sense, but I still don't understand why getting/installing swagger in configure_dev-deps.sh in the previous iteration worked and it didn't adding it to go.mod.

Can you explain this a bit further please?

@tsachiherman
Copy link
Contributor Author

tsachiherman commented Nov 23, 2020 •
edited
Loading

@tsachiherman The changes make sense, but I still don't understand why getting/installing swagger in configure_dev-deps.sh in the previous iteration worked and it didn't adding it to go.mod.

Can you explain this a bit further please?

Yes, of course. The scripts/configure_dev-deps.sh was doing a cd && GO111MODULE=on go get "$1@${VERSION}" 2>&1 call. Note the cd, which would change the current directory to the user's home directory inside the subshell.
This means that any go.mod would be looked at the user's home directory.

I think that we need to address this as well, but that's already a separate issue.

@btoll
Copy link

btoll commented Nov 23, 2020

@tsachiherman The changes make sense, but I still don't understand why getting/installing swagger in configure_dev-deps.sh in the previous iteration worked and it didn't adding it to go.mod.
Can you explain this a bit further please?

Yes, of course. The scripts/configure_dev-deps.sh was doing a cd && GO111MODULE=on go get "$1@${VERSION}" 2>&1 call. Note the cd, which would change the current directory to the user's home directory inside the subshell.
This means that any go.mod would be looked at the user's home directory.

I think that we need to address this as well, but that's already a separate issue.

Ok, I see. Thanks for jumping on the ticket.

@tsachiherman tsachiherman merged commit 6e7776e into algorand:master Nov 23, 2020
@tsachiherman tsachiherman deleted the tsachi/swagger_go_mod branch November 23, 2020 21:35
@tsachiherman tsachiherman mentioned this pull request Jan 19, 2021
Merged
tsachiherman added a commit that referenced this pull request Jan 20, 2021
@tsachiherman
Fix regression to loadTelemetryConfig introduced in 2.3.0 (#1845)
3e2d7ce 
As part of a refactoring effort in #1709, a small bug was introduced that changed the intended behavior of `loadTelemetryConfig`. This PR restores the previous behavior and adds a unit test for that.
tsachiherman added a commit to tsachiherman/go-algorand that referenced this pull request Jul 7, 2021
@tsachiherman
Derive the swagger version from the go.mod file (algorand#1709)
356a920 
Derive the swagger version from the go.mod file
tsachiherman added a commit to tsachiherman/go-algorand that referenced this pull request Jul 7, 2021
@tsachiherman
Fix regression to loadTelemetryConfig introduced in 2.3.0 (algorand#1845
6e12e0e 
)

As part of a refactoring effort in algorand#1709, a small bug was introduced that changed the intended behavior of `loadTelemetryConfig`. This PR restores the previous behavior and adds a unit test for that.
tsachiherman added a commit to tsachiherman/go-algorand that referenced this pull request Jul 7, 2021
@tsachiherman @onetechnical
Fix regression to loadTelemetryConfig introduced in 2.3.0 (algorand#1845
6b37d16 
)

As part of a refactoring effort in algorand#1709, a small bug was introduced that changed the intended behavior of `loadTelemetryConfig`. This PR restores the previous behavior and adds a unit test for that.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@egieseke egieseke egieseke left review comments

@btoll btoll btoll approved these changes

@onetechnical onetechnical Awaiting requested review from onetechnical

Assignees

@tsachiherman tsachiherman

Labels
Infrastructure
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tsachiherman @btoll @egieseke