You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "tanmayrauth (via GitHub)" <gi...@apache.org> on 2026/05/11 17:55:13 UTC
[PR] feat(cli): add upgrade and rollback commands [iceberg-go]
tanmayrauth opened a new pull request, #1071:
URL: https://github.com/apache/iceberg-go/pull/1071
Add `iceberg upgrade TABLE_ID VERSION` with --dry-run and --yes. Add `iceberg rollback TABLE_ID --snapshot-id ID` with --yes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(cli): add upgrade and rollback commands [iceberg-go]
Posted by "tanmayrauth (via GitHub)" <gi...@apache.org>.
tanmayrauth commented on PR #1071:
URL: https://github.com/apache/iceberg-go/pull/1071#issuecomment-4445506229
Thanks for the thorough review! I've made the changes:
- Rollback now goes through a new Transaction.RollbackToSnapshot method that bakes in both the CAS (AssertRefSnapshotID) and ancestry check (IsAncestorOf), so it gets the OCC retry loop from Transaction.Commit for free. Also kept the early ancestry check in the CLI for a clearer user-facing error before hitting the confirmation prompt.
- Moved the version guard after the dry-run branch so upgrade TABLE 1 --dry-run produces the no-op preview as expected.
- Fixed receivers, pipe error handling, and pterm cleanup as suggested.
- Added end-to-end tests with a panicking fake catalog covering dry-run-no-commit, already-at-version dry-run, non-TTY rejection, and non-ancestor rejection.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(cli): add upgrade and rollback commands [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka merged PR #1071:
URL: https://github.com/apache/iceberg-go/pull/1071
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(cli): add upgrade and rollback commands [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on PR #1071:
URL: https://github.com/apache/iceberg-go/pull/1071#issuecomment-4444555606
The `upgrade` path is clean — `tx.UpgradeFormatVersion` + `tx.Commit` is the right surface and the dry-run/confirmation flow matches the rest of the series.
The concerns are concentrated in `runRollback`. It drops to `cat.CommitTable` directly with only `AssertTableUUID`, so a concurrent commit advancing `main` between the load and the commit is silently overwritten — both Java's `SetSnapshotOperation` and PyIceberg's `_set_ref_snapshot` always include `AssertRefSnapshotID(main, current)`, and `previousSnapshotID` is already computed here so threading it into the requirements is a small change. The same function also doesn't check that the target snapshot is an ancestor of current main, so today the command will happily re-point `main` to an unrelated snapshot from a different branch history — Java's `rollbackTo` and PyIceberg's `rollback_to_snapshot` both reject non-ancestors, and `table.IsAncestorOf` is already exported.
Tests cover formatters only; nothing exercises `runRollback`/`runUpgrade` themselves, so the dry-run-no-commit and non-TTY-without-`--yes` lines from #957 are untested. A fake catalog whose `CommitTable` panics would cover that without Docker.
Inline comments left on the specific lines. Once the rollback CAS + ancestry checks land this should be ready.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(cli): add upgrade and rollback commands [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on code in PR #1071:
URL: https://github.com/apache/iceberg-go/pull/1071#discussion_r3236917684
##########
cmd/iceberg/upgrade_rollback.go:
##########
@@ -19,23 +19,144 @@ package main
import (
"context"
- "errors"
+ "encoding/json"
+ "fmt"
"os"
+ "strconv"
"github.com/apache/iceberg-go/catalog"
+ "github.com/apache/iceberg-go/table"
+ "github.com/pterm/pterm"
)
-func runUpgrade(_ context.Context, output Output, _ catalog.Catalog, _ *UpgradeCmd) {
- output.Error(errors.New("upgrade: not yet implemented"))
- os.Exit(1)
+func runUpgrade(ctx context.Context, output Output, cat catalog.Catalog, cmd *UpgradeCmd) {
+ tbl := loadTable(ctx, output, cat, cmd.TableID)
+ meta := tbl.Metadata()
+ currentVersion := meta.Version()
+
+ if cmd.FormatVersion <= currentVersion {
+ output.Error(fmt.Errorf("target format version %d must be greater than current version %d",
+ cmd.FormatVersion, currentVersion))
+ os.Exit(1)
+ }
+
+ result := UpgradeResult{
+ DryRun: cmd.DryRun,
+ Table: tableIDString(tbl),
+ PreviousVersion: currentVersion,
+ TargetVersion: cmd.FormatVersion,
+ SpecURL: specURL(cmd.FormatVersion),
+ }
+
+ if cmd.DryRun {
+ output.UpgradeResult(result)
+
+ return
+ }
+
+ prompt := fmt.Sprintf("Upgrade %s from format version %d to %d?",
+ tableIDString(tbl), currentVersion, cmd.FormatVersion)
+ if err := confirmAction(prompt, cmd.Yes); err != nil {
+ output.Error(err)
+ os.Exit(1)
+ }
+
+ tx := tbl.NewTransaction()
+ if err := tx.UpgradeFormatVersion(cmd.FormatVersion); err != nil {
+ output.Error(fmt.Errorf("upgrade failed: %w", err))
+ os.Exit(1)
+ }
+
+ if _, err := tx.Commit(ctx); err != nil {
+ output.Error(fmt.Errorf("commit failed: %w", err))
+ os.Exit(1)
+ }
+
+ output.UpgradeResult(result)
+}
+
+func runRollback(ctx context.Context, output Output, cat catalog.Catalog, cmd *RollbackCmd) {
+ tbl := loadTable(ctx, output, cat, cmd.TableID)
+ meta := tbl.Metadata()
+
+ snap := meta.SnapshotByID(cmd.SnapshotID)
+ if snap == nil {
+ output.Error(fmt.Errorf("snapshot %d not found in table %s", cmd.SnapshotID, tableIDString(tbl)))
+ os.Exit(1)
+ }
+
+ var previousSnapshotID *int64
+ if cs := meta.CurrentSnapshot(); cs != nil {
+ id := cs.SnapshotID
+ previousSnapshotID = &id
+ }
+
+ prompt := fmt.Sprintf("Roll back %s to snapshot %d?", tableIDString(tbl), cmd.SnapshotID)
+ if err := confirmAction(prompt, cmd.Yes); err != nil {
+ output.Error(err)
+ os.Exit(1)
+ }
+
+ update := table.NewSetSnapshotRefUpdate(table.MainBranch, cmd.SnapshotID, table.BranchRef, 0, 0, 0)
+ reqs := []table.Requirement{table.AssertTableUUID(meta.TableUUID())}
Review Comment:
Requirements slice is just `AssertTableUUID`. A concurrent writer advancing `main` between `loadTable` and `CommitTable` will be silently clobbered. `previousSnapshotID` is already computed for the result struct — pass it through as `table.AssertRefSnapshotID(table.MainBranch, previousSnapshotID)` here:
```go
reqs := []table.Requirement{
table.AssertTableUUID(meta.TableUUID()),
table.AssertRefSnapshotID(table.MainBranch, previousSnapshotID),
}
```
PyIceberg's `_set_ref_snapshot` does this unconditionally; both Java's `SetSnapshotOperation` and the iceberg-go transaction layer use the same CAS pattern. Without it, this rollback is destructive under any multi-writer setup.
##########
cmd/iceberg/upgrade_rollback.go:
##########
@@ -19,23 +19,144 @@ package main
import (
"context"
- "errors"
+ "encoding/json"
+ "fmt"
"os"
+ "strconv"
"github.com/apache/iceberg-go/catalog"
+ "github.com/apache/iceberg-go/table"
+ "github.com/pterm/pterm"
)
-func runUpgrade(_ context.Context, output Output, _ catalog.Catalog, _ *UpgradeCmd) {
- output.Error(errors.New("upgrade: not yet implemented"))
- os.Exit(1)
+func runUpgrade(ctx context.Context, output Output, cat catalog.Catalog, cmd *UpgradeCmd) {
+ tbl := loadTable(ctx, output, cat, cmd.TableID)
+ meta := tbl.Metadata()
+ currentVersion := meta.Version()
+
+ if cmd.FormatVersion <= currentVersion {
+ output.Error(fmt.Errorf("target format version %d must be greater than current version %d",
+ cmd.FormatVersion, currentVersion))
+ os.Exit(1)
+ }
+
+ result := UpgradeResult{
+ DryRun: cmd.DryRun,
+ Table: tableIDString(tbl),
+ PreviousVersion: currentVersion,
+ TargetVersion: cmd.FormatVersion,
+ SpecURL: specURL(cmd.FormatVersion),
+ }
+
+ if cmd.DryRun {
+ output.UpgradeResult(result)
+
+ return
+ }
+
+ prompt := fmt.Sprintf("Upgrade %s from format version %d to %d?",
+ tableIDString(tbl), currentVersion, cmd.FormatVersion)
+ if err := confirmAction(prompt, cmd.Yes); err != nil {
+ output.Error(err)
+ os.Exit(1)
+ }
+
+ tx := tbl.NewTransaction()
+ if err := tx.UpgradeFormatVersion(cmd.FormatVersion); err != nil {
+ output.Error(fmt.Errorf("upgrade failed: %w", err))
+ os.Exit(1)
+ }
+
+ if _, err := tx.Commit(ctx); err != nil {
+ output.Error(fmt.Errorf("commit failed: %w", err))
+ os.Exit(1)
+ }
+
+ output.UpgradeResult(result)
+}
+
+func runRollback(ctx context.Context, output Output, cat catalog.Catalog, cmd *RollbackCmd) {
+ tbl := loadTable(ctx, output, cat, cmd.TableID)
+ meta := tbl.Metadata()
+
+ snap := meta.SnapshotByID(cmd.SnapshotID)
+ if snap == nil {
+ output.Error(fmt.Errorf("snapshot %d not found in table %s", cmd.SnapshotID, tableIDString(tbl)))
+ os.Exit(1)
+ }
+
+ var previousSnapshotID *int64
+ if cs := meta.CurrentSnapshot(); cs != nil {
+ id := cs.SnapshotID
+ previousSnapshotID = &id
+ }
+
+ prompt := fmt.Sprintf("Roll back %s to snapshot %d?", tableIDString(tbl), cmd.SnapshotID)
+ if err := confirmAction(prompt, cmd.Yes); err != nil {
+ output.Error(err)
+ os.Exit(1)
+ }
+
+ update := table.NewSetSnapshotRefUpdate(table.MainBranch, cmd.SnapshotID, table.BranchRef, 0, 0, 0)
+ reqs := []table.Requirement{table.AssertTableUUID(meta.TableUUID())}
+
+ if _, _, err := cat.CommitTable(ctx, tbl.Identifier(), reqs, []table.Update{update}); err != nil {
Review Comment:
Going through `cat.CommitTable` directly here (rather than `tbl.NewTransaction()` like `runUpgrade` does) also drops the OCC retry loop and `rewriteRefSnapshotRequirements` that `Transaction.Commit` provides. Two options: lift this through a new `Transaction.RollbackToSnapshot(ctx, id)` that bakes in the CAS + ancestry checks (preferred — mirrors the `UpgradeFormatVersion` pattern), or leave a `TODO` here noting the asymmetry for a follow-up.
##########
cmd/iceberg/upgrade_rollback_test.go:
##########
@@ -0,0 +1,166 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package main
+
+import (
+ "bytes"
+ "os"
+ "testing"
+
+ "github.com/pterm/pterm"
+ "github.com/stretchr/testify/assert"
+)
+
+func TestSpecURL(t *testing.T) {
+ assert.Contains(t, specURL(1), "version-1")
+ assert.Contains(t, specURL(2), "version-2")
+ assert.Contains(t, specURL(3), "version-3")
+ assert.Equal(t, "https://iceberg.apache.org/spec/", specURL(99))
+}
+
+func TestTextOutputUpgradeResultDryRun(t *testing.T) {
+ var buf bytes.Buffer
+ pterm.SetDefaultOutput(&buf)
Review Comment:
`pterm.SetDefaultOutput(&buf)` mutates package-global state without restore (same pattern at lines 60, 107, 127). Fine while everything's sequential, but the moment any test adds `t.Parallel()` the output sink is a data race. Add `t.Cleanup(func() { pterm.SetDefaultOutput(os.Stdout); pterm.EnableColor() })` per test.
##########
cmd/iceberg/upgrade_rollback.go:
##########
@@ -19,23 +19,144 @@ package main
import (
"context"
- "errors"
+ "encoding/json"
+ "fmt"
"os"
+ "strconv"
"github.com/apache/iceberg-go/catalog"
+ "github.com/apache/iceberg-go/table"
+ "github.com/pterm/pterm"
)
-func runUpgrade(_ context.Context, output Output, _ catalog.Catalog, _ *UpgradeCmd) {
- output.Error(errors.New("upgrade: not yet implemented"))
- os.Exit(1)
+func runUpgrade(ctx context.Context, output Output, cat catalog.Catalog, cmd *UpgradeCmd) {
+ tbl := loadTable(ctx, output, cat, cmd.TableID)
+ meta := tbl.Metadata()
+ currentVersion := meta.Version()
+
+ if cmd.FormatVersion <= currentVersion {
Review Comment:
The `cmd.FormatVersion <= currentVersion` guard fires before the dry-run branch, so `upgrade TABLE 1 --dry-run` on a v2 table exits 1 instead of producing the promised `[DRY RUN]` no-op preview. Move the guard inside `!cmd.DryRun`, or render a "no-op, already at version N" dry-run line. The dry-run contract in #957 requires exit 0 with a preview.
##########
cmd/iceberg/upgrade_rollback.go:
##########
@@ -19,23 +19,144 @@ package main
import (
"context"
- "errors"
+ "encoding/json"
+ "fmt"
"os"
+ "strconv"
"github.com/apache/iceberg-go/catalog"
+ "github.com/apache/iceberg-go/table"
+ "github.com/pterm/pterm"
)
-func runUpgrade(_ context.Context, output Output, _ catalog.Catalog, _ *UpgradeCmd) {
- output.Error(errors.New("upgrade: not yet implemented"))
- os.Exit(1)
+func runUpgrade(ctx context.Context, output Output, cat catalog.Catalog, cmd *UpgradeCmd) {
+ tbl := loadTable(ctx, output, cat, cmd.TableID)
+ meta := tbl.Metadata()
+ currentVersion := meta.Version()
+
+ if cmd.FormatVersion <= currentVersion {
+ output.Error(fmt.Errorf("target format version %d must be greater than current version %d",
+ cmd.FormatVersion, currentVersion))
+ os.Exit(1)
+ }
+
+ result := UpgradeResult{
+ DryRun: cmd.DryRun,
+ Table: tableIDString(tbl),
+ PreviousVersion: currentVersion,
+ TargetVersion: cmd.FormatVersion,
+ SpecURL: specURL(cmd.FormatVersion),
+ }
+
+ if cmd.DryRun {
+ output.UpgradeResult(result)
+
+ return
+ }
+
+ prompt := fmt.Sprintf("Upgrade %s from format version %d to %d?",
+ tableIDString(tbl), currentVersion, cmd.FormatVersion)
+ if err := confirmAction(prompt, cmd.Yes); err != nil {
+ output.Error(err)
+ os.Exit(1)
+ }
+
+ tx := tbl.NewTransaction()
+ if err := tx.UpgradeFormatVersion(cmd.FormatVersion); err != nil {
+ output.Error(fmt.Errorf("upgrade failed: %w", err))
+ os.Exit(1)
+ }
+
+ if _, err := tx.Commit(ctx); err != nil {
+ output.Error(fmt.Errorf("commit failed: %w", err))
+ os.Exit(1)
+ }
+
+ output.UpgradeResult(result)
+}
+
+func runRollback(ctx context.Context, output Output, cat catalog.Catalog, cmd *RollbackCmd) {
+ tbl := loadTable(ctx, output, cat, cmd.TableID)
+ meta := tbl.Metadata()
+
+ snap := meta.SnapshotByID(cmd.SnapshotID)
+ if snap == nil {
+ output.Error(fmt.Errorf("snapshot %d not found in table %s", cmd.SnapshotID, tableIDString(tbl)))
+ os.Exit(1)
+ }
+
+ var previousSnapshotID *int64
+ if cs := meta.CurrentSnapshot(); cs != nil {
+ id := cs.SnapshotID
+ previousSnapshotID = &id
+ }
+
+ prompt := fmt.Sprintf("Roll back %s to snapshot %d?", tableIDString(tbl), cmd.SnapshotID)
+ if err := confirmAction(prompt, cmd.Yes); err != nil {
+ output.Error(err)
+ os.Exit(1)
+ }
+
+ update := table.NewSetSnapshotRefUpdate(table.MainBranch, cmd.SnapshotID, table.BranchRef, 0, 0, 0)
+ reqs := []table.Requirement{table.AssertTableUUID(meta.TableUUID())}
+
+ if _, _, err := cat.CommitTable(ctx, tbl.Identifier(), reqs, []table.Update{update}); err != nil {
+ output.Error(fmt.Errorf("rollback failed: %w", err))
+ os.Exit(1)
+ }
+
+ result := RollbackResult{
+ Table: tableIDString(tbl),
+ PreviousSnapshotID: previousSnapshotID,
+ RolledBackToSnapshotID: cmd.SnapshotID,
+ }
+
+ output.RollbackResult(result)
}
-func runRollback(_ context.Context, output Output, _ catalog.Catalog, _ *RollbackCmd) {
- output.Error(errors.New("rollback: not yet implemented"))
- os.Exit(1)
+func specURL(version int) string {
+ switch version {
+ case 1:
+ return "https://iceberg.apache.org/spec/#version-1-analytic-data-tables"
+ case 2:
+ return "https://iceberg.apache.org/spec/#version-2-row-level-deletes"
+ case 3:
+ return "https://iceberg.apache.org/spec/#version-3-extended-types-and-features"
+ default:
+ return "https://iceberg.apache.org/spec/"
+ }
+}
+
+func (t textOutput) UpgradeResult(result UpgradeResult) {
Review Comment:
Receiver named `t` but unused, and `t` universally means `*testing.T` in this `package main` where tests live alongside production code. The established pattern in `output.go` is a blank receiver for methods that don't read the receiver: `func (textOutput) UpgradeResult(...)`. Same applies to `RollbackResult` below.
##########
cmd/iceberg/upgrade_rollback.go:
##########
@@ -19,23 +19,144 @@ package main
import (
"context"
- "errors"
+ "encoding/json"
+ "fmt"
"os"
+ "strconv"
"github.com/apache/iceberg-go/catalog"
+ "github.com/apache/iceberg-go/table"
+ "github.com/pterm/pterm"
)
-func runUpgrade(_ context.Context, output Output, _ catalog.Catalog, _ *UpgradeCmd) {
- output.Error(errors.New("upgrade: not yet implemented"))
- os.Exit(1)
+func runUpgrade(ctx context.Context, output Output, cat catalog.Catalog, cmd *UpgradeCmd) {
+ tbl := loadTable(ctx, output, cat, cmd.TableID)
+ meta := tbl.Metadata()
+ currentVersion := meta.Version()
+
+ if cmd.FormatVersion <= currentVersion {
+ output.Error(fmt.Errorf("target format version %d must be greater than current version %d",
+ cmd.FormatVersion, currentVersion))
+ os.Exit(1)
+ }
+
+ result := UpgradeResult{
+ DryRun: cmd.DryRun,
+ Table: tableIDString(tbl),
+ PreviousVersion: currentVersion,
+ TargetVersion: cmd.FormatVersion,
+ SpecURL: specURL(cmd.FormatVersion),
+ }
+
+ if cmd.DryRun {
+ output.UpgradeResult(result)
+
+ return
+ }
+
+ prompt := fmt.Sprintf("Upgrade %s from format version %d to %d?",
+ tableIDString(tbl), currentVersion, cmd.FormatVersion)
+ if err := confirmAction(prompt, cmd.Yes); err != nil {
+ output.Error(err)
+ os.Exit(1)
+ }
+
+ tx := tbl.NewTransaction()
+ if err := tx.UpgradeFormatVersion(cmd.FormatVersion); err != nil {
+ output.Error(fmt.Errorf("upgrade failed: %w", err))
+ os.Exit(1)
+ }
+
+ if _, err := tx.Commit(ctx); err != nil {
+ output.Error(fmt.Errorf("commit failed: %w", err))
+ os.Exit(1)
+ }
+
+ output.UpgradeResult(result)
+}
+
+func runRollback(ctx context.Context, output Output, cat catalog.Catalog, cmd *RollbackCmd) {
+ tbl := loadTable(ctx, output, cat, cmd.TableID)
+ meta := tbl.Metadata()
+
+ snap := meta.SnapshotByID(cmd.SnapshotID)
+ if snap == nil {
+ output.Error(fmt.Errorf("snapshot %d not found in table %s", cmd.SnapshotID, tableIDString(tbl)))
+ os.Exit(1)
+ }
Review Comment:
`meta.SnapshotByID(cmd.SnapshotID) != nil` proves the snapshot exists but not that it's on the current main ancestry chain. As written, `rollback --snapshot-id <sibling-branch-head>` succeeds and re-points `main` at an unrelated history — downstream readers see a `snapshot-log` with a gap. Java's `ManageSnapshots.rollbackTo` and PyIceberg's `rollback_to_snapshot` both reject non-ancestors before committing.
`table.IsAncestorOf` is already exported from `table/snapshot_ancestry.go`. Suggested:
```go
if cs := meta.CurrentSnapshot(); cs != nil {
if !table.IsAncestorOf(cs.SnapshotID, cmd.SnapshotID, meta.SnapshotByID) {
output.Error(fmt.Errorf("snapshot %d is not an ancestor of current snapshot %d", cmd.SnapshotID, cs.SnapshotID))
os.Exit(1)
}
}
```
##########
cmd/iceberg/upgrade_rollback_test.go:
##########
@@ -0,0 +1,166 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package main
+
+import (
+ "bytes"
+ "os"
+ "testing"
+
+ "github.com/pterm/pterm"
+ "github.com/stretchr/testify/assert"
+)
+
+func TestSpecURL(t *testing.T) {
+ assert.Contains(t, specURL(1), "version-1")
+ assert.Contains(t, specURL(2), "version-2")
+ assert.Contains(t, specURL(3), "version-3")
+ assert.Equal(t, "https://iceberg.apache.org/spec/", specURL(99))
+}
+
+func TestTextOutputUpgradeResultDryRun(t *testing.T) {
+ var buf bytes.Buffer
+ pterm.SetDefaultOutput(&buf)
+ pterm.DisableColor()
+
+ result := UpgradeResult{
+ DryRun: true,
+ Table: "db.events",
+ PreviousVersion: 1,
+ TargetVersion: 2,
+ SpecURL: specURL(2),
+ }
+
+ buf.Reset()
+ textOutput{}.UpgradeResult(result)
+
+ output := buf.String()
+ assert.Contains(t, output, "[DRY RUN]")
+ assert.Contains(t, output, "format version 1 to 2")
+ assert.Contains(t, output, "version-2")
+}
+
+func TestTextOutputUpgradeResultCommitted(t *testing.T) {
+ var buf bytes.Buffer
+ pterm.SetDefaultOutput(&buf)
+ pterm.DisableColor()
+
+ result := UpgradeResult{
+ DryRun: false,
+ Table: "db.events",
+ PreviousVersion: 1,
+ TargetVersion: 2,
+ SpecURL: specURL(2),
+ }
+
+ buf.Reset()
+ textOutput{}.UpgradeResult(result)
+
+ output := buf.String()
+ assert.Contains(t, output, "Upgraded db.events")
+ assert.Contains(t, output, "format version 1 to 2")
+}
+
+func TestJSONOutputUpgradeResult(t *testing.T) {
+ oldStdout := os.Stdout
+ r, w, _ := os.Pipe()
Review Comment:
`r, w, _ := os.Pipe()` swallows the error; on pipe-creation failure, `w` is nil and the subsequent `os.Stdout = w` corrupts the process for any later test. Same issue in `TestJSONOutputRollbackResult` at line 145. Suggested:
```go
r, w, err := os.Pipe()
require.NoError(t, err)
oldStdout := os.Stdout
os.Stdout = w
t.Cleanup(func() { w.Close(); os.Stdout = oldStdout })
```
##########
cmd/iceberg/upgrade_rollback_test.go:
##########
@@ -0,0 +1,166 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package main
+
+import (
+ "bytes"
+ "os"
+ "testing"
+
+ "github.com/pterm/pterm"
+ "github.com/stretchr/testify/assert"
+)
+
+func TestSpecURL(t *testing.T) {
+ assert.Contains(t, specURL(1), "version-1")
+ assert.Contains(t, specURL(2), "version-2")
+ assert.Contains(t, specURL(3), "version-3")
+ assert.Equal(t, "https://iceberg.apache.org/spec/", specURL(99))
+}
+
+func TestTextOutputUpgradeResultDryRun(t *testing.T) {
+ var buf bytes.Buffer
+ pterm.SetDefaultOutput(&buf)
+ pterm.DisableColor()
+
+ result := UpgradeResult{
+ DryRun: true,
+ Table: "db.events",
+ PreviousVersion: 1,
+ TargetVersion: 2,
+ SpecURL: specURL(2),
+ }
+
+ buf.Reset()
+ textOutput{}.UpgradeResult(result)
+
+ output := buf.String()
+ assert.Contains(t, output, "[DRY RUN]")
+ assert.Contains(t, output, "format version 1 to 2")
+ assert.Contains(t, output, "version-2")
+}
+
+func TestTextOutputUpgradeResultCommitted(t *testing.T) {
+ var buf bytes.Buffer
+ pterm.SetDefaultOutput(&buf)
+ pterm.DisableColor()
+
+ result := UpgradeResult{
+ DryRun: false,
+ Table: "db.events",
+ PreviousVersion: 1,
+ TargetVersion: 2,
+ SpecURL: specURL(2),
+ }
+
+ buf.Reset()
+ textOutput{}.UpgradeResult(result)
+
+ output := buf.String()
+ assert.Contains(t, output, "Upgraded db.events")
+ assert.Contains(t, output, "format version 1 to 2")
+}
+
+func TestJSONOutputUpgradeResult(t *testing.T) {
Review Comment:
Coverage here is formatter-only — neither `runRollback` nor `runUpgrade` is exercised end-to-end. The two contract lines from #957 ("verify no catalog commit in dry-run", "non-TTY without `--yes` errors") aren't tested. A fake `catalog.Catalog` whose `CommitTable` panics would catch the dry-run violation without needing Docker.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org