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

all: Bump supported Go versions to 1.19 and 1.20. #4810

Merged
merged 2 commits into from
Mar 22, 2023

Conversation

Shaptic
Copy link
Contributor

@Shaptic Shaptic commented Mar 15, 2023

It's time 👀

Also, stop using specific versions so that we always used the latest patch version of a major release. Didn't this used to be the case?

@Shaptic Shaptic marked this pull request as ready for review March 16, 2023 00:17
@Shaptic Shaptic requested a review from a team March 16, 2023 00:17
Copy link
Contributor

@tsachiherman tsachiherman 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 that it's not as "safe".
i.e. existing code would stop working / work differently at some point, and we won't know why.
example for that is the govet tool; once in a while, it's being extended, and it starts generating new warnings. Having warning on new introduced code is always ok, but having existing code start failing ( without any hint why ) is a bit questionable.

@tsachiherman
Copy link
Contributor

I do agree, though, that we should align with the latest. I'm not sure though if we want this to be automatic or not.

@Shaptic
Copy link
Contributor Author

Shaptic commented Mar 16, 2023

@tsachiherman hmm, can you explain what you mean about "existing code starts failing"? All of the CI checks pass with the new versions, incl. linting, so any future failures would have to come from new code.

@Shaptic
Copy link
Contributor Author

Shaptic commented Mar 16, 2023

(For context, we are committed to supporting the two latest major versions of Go, so this is something we do regularly with new releases [e.g. see #4499].)

@tsachiherman
Copy link
Contributor

@tsachiherman hmm, can you explain what you mean about "existing code starts failing"? All of the CI checks pass with the new versions, incl. linting, so any future failures would have to come from new code.

sure. The CI checks are only when we actively trigger a build, not when a new go version is introduced.
so, if you have a PR that is failing, you wouldn't know if it's related to the go version or to the code in that PR.

@tsachiherman
Copy link
Contributor

This isn't expected to be a common issue, but I've seen in happening before. ( specifically, between go 1.11.3 to 1.11.6, when the DNS SRV record processing logic was modified ).

@Shaptic
Copy link
Contributor Author

Shaptic commented Mar 16, 2023

Ah I see. That's an interesting point, but if it was definitively related to the version then you would see e.g. success on 1.19 and failure on 1.20, at least in places we have the matrix.

@tsachiherman
Copy link
Contributor

How do you feel around using your pr as is, but also adding some validation that we're using a specific sub version. That way, we're always need to update that explicitly.

@mollykarcher
Copy link
Contributor

mollykarcher commented Mar 21, 2023

If I understand your point Tsachi, your concern is that if we have "1.20" here and under the hood the version changes from 1.20.1 -> 1.20.2 which causes some test failures, it won't be obvious to the developer that the version changed at all, so it would be tough to nail down the cause for the failure. I get that, but it seems like it would be a pretty rare occurrence to have breaking changes come in with new patch versions.

This behavior mirrors what both quickstart and the package builders do, and I think we greatly benefit from being consistent here. But you could question whether or not we should be pinning versions in those places as well. However, I would argue against this because it could make our reaction to security patch updates easier; we could simply rebuild/redeploy the same version without updating any source code, rather that going through the entire bump/re-release/upgrade process.

@Shaptic Shaptic merged commit 5c371b9 into stellar:soroban-xdr-next Mar 22, 2023
@Shaptic Shaptic deleted the bump-golang branch March 22, 2023 23:47
Shaptic added a commit that referenced this pull request Mar 23, 2023
* services/horizon: Add `account_credited`/`debited` effects for SAC events. (#4806)
* Include contract asset balance changes in payments  (#4807)
* all: Bump supported Go versions to 1.19 and 1.20. (#4810)
* Use version 0.7.0 and 8abd3353c728f09ee1c8a2544f67a853e915afc2 of rs-soroban-sdk dependency
* Embed XDR into a separate file for testing

---------

Co-authored-by: shawn <[email protected]>
Co-authored-by: tamirms <[email protected]>
paulbellamy pushed a commit that referenced this pull request Mar 27, 2023
commit 1caf12e
Author: Paul Bellamy <[email protected]>
Date:   Thu Mar 23 11:02:08 2023 +0000

    474/soroban diagnostic events (#4813)

    * Add captive core config for enable soroban diagnostic events

    * Add a horizon flag for --captive-core-enable-soroban-diagnostic-events

    * Revert "Add a horizon flag for --captive-core-enable-soroban-diagnostic-events"

    This reverts commit fd5845c.

    * Revert ingest/ledgerbackend flags. It can just come from the toml

    * Remove flag from `ingest/ledgerbackend/toml.go`

commit 5c371b9
Author: George <[email protected]>
Date:   Wed Mar 22 16:46:57 2023 -0700

    all: Bump supported Go versions to 1.19 and 1.20. (#4810)

    * Yeet staticcheck upgrade

commit 380355e
Author: shawn <[email protected]>
Date:   Wed Mar 22 09:19:49 2023 -0700

    #4728: include contract asset balance changes in payments  (#4807)

commit 107d5d1
Author: George <[email protected]>
Date:   Mon Mar 20 18:52:17 2023 -0700

    services/horizon: Add `account_credited`/`debited` effects for SAC events. (#4806)

    * Harden amount parser
    * Propogate network passphrase and add tests
    * Integration tests check effect values, passing 🙏
    * Move stringification to xdr package
    * Separate to/from addresses in contract events
2opremio added a commit that referenced this pull request Mar 27, 2023
* Gofmt

* update xdr to 7356dc237ee0db5626561c129fb3fa4beaabbac6

* Soroban xdr value overhaul ScVal.Equals fixes (#4815)

* Fix a bug in generated xdr for boolean unmarshalling

* Output more helpful message when scval test fails

* Fix equality for xdr ScVal overhaul

* use bytes.equal

* soroban-xdr-next-next: Fix txnbuild tests (#4817)

* soroban-xdr-next-next: Fix XDR bool encoding/decoding (#4818)

* all: Update libs & tests to support `ScVal`ue overhaul. (#4819)

* services/horizon: Add `account_credited`/`debited` effects for SAC events. (#4806)
* Include contract asset balance changes in payments  (#4807)
* all: Bump supported Go versions to 1.19 and 1.20. (#4810)
* Use version 0.7.0 and 8abd3353c728f09ee1c8a2544f67a853e915afc2 of rs-soroban-sdk dependency
* Embed XDR into a separate file for testing

---------

Co-authored-by: shawn <[email protected]>
Co-authored-by: tamirms <[email protected]>

* ingest: Extract diagnostic events in GetOperationEvents() (#4820)

---------

Co-authored-by: Alfonso Acosta <[email protected]>
Co-authored-by: George <[email protected]>
Co-authored-by: shawn <[email protected]>
Co-authored-by: tamirms <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants