Skip to content

fix: promote cmd should sync app-interface first before making change #837

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

Open
feichashao wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: promote cmd should sync app-interface first before making change #837

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 4 additions & 6 deletions cmd/promote/git/app_interface.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,9 +266,8 @@ func GetCurrentPackageTagFromAppInterface(saasFile string) (string, error) {

func (a *AppInterface) UpdateAppInterface(_, saasFile, currentGitHash, promotionGitHash, branchName string, hotfix bool) error {

if err := a.GitExecutor.Run(a.GitDirectory, "git", "checkout", "master"); err != nil {
return fmt.Errorf("failed to checkout master: branch %v", err)
}
// Note: Caller should have already synced with origin/master before calling this function
// to ensure currentGitHash is accurate

if err := a.GitExecutor.Run(a.GitDirectory, "git", "branch", "-D", branchName); err != nil {
fmt.Printf("failed to cleanup branch %s: %v, continuing to create it.\n", branchName, err)
Expand Down Expand Up @@ -312,9 +311,8 @@ func (a *AppInterface) UpdateAppInterface(_, saasFile, currentGitHash, promotion

func (a *AppInterface) UpdatePackageTag(saasFile, oldTag, promotionTag, branchName string) error {

if err := a.GitExecutor.Run(a.GitDirectory, "git", "checkout", "master"); err != nil {
return fmt.Errorf("failed to checkout master branch: %v", err)
}
// Note: Caller should have already synced with origin/master before calling this function
// to ensure oldTag is accurate

if err := a.GitExecutor.Run(a.GitDirectory, "git", "branch", "-D", branchName); err != nil {
fmt.Printf("failed to cleanup branch %s: %v, continuing to create it.\n", branchName, err)
Expand Down
38 changes: 9 additions & 29 deletions cmd/promote/git/app_interface_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ import (
"github.com/stretchr/testify/require"
)

// MockExec is a mock implementation of iexec.IExec for testing
// It is exported so other packages can use it in their tests
type MockExec struct {
mock.Mock
iexec.IExec
Expand Down Expand Up @@ -367,7 +369,6 @@ func TestUpdatePackageTag(t *testing.T) {

_ = os.WriteFile(saasFile, []byte("tag: old123"), 0o600)

mockExec.On("Run", tmpDir, "git", []string{"checkout", "master"}).Return(nil).Once()
mockExec.On("Run", tmpDir, "git", []string{"branch", "-D", "feature-branch"}).Return(errors.New("branch does not exist")).Once()

return AppInterface{
Expand All @@ -378,33 +379,13 @@ func TestUpdatePackageTag(t *testing.T) {
expectedErr: "",
expectedFile: "tag: new456",
},
{
name: "fails_git_checkout",
setup: func(t *testing.T) (AppInterface, string) {
mockExec := new(MockExec)
tmpDir := t.TempDir()
saasFile := filepath.Join(tmpDir, "test.yaml")

_ = os.WriteFile(saasFile, []byte("tag: old123"), 0o600)

mockExec.On("Run", tmpDir, "git", []string{"checkout", "master"}).Return(errors.New("checkout failed")).Once()

return AppInterface{
GitDirectory: tmpDir,
GitExecutor: mockExec,
}, saasFile
},
expectedErr: "failed to checkout master branch",
expectedFile: "tag: old123",
},
{
name: "fails_reading_file",
setup: func(t *testing.T) (AppInterface, string) {
mockExec := new(MockExec)
tmpDir := t.TempDir()
saasFile := filepath.Join(tmpDir, "nonexistent.yaml")

mockExec.On("Run", tmpDir, "git", []string{"checkout", "master"}).Return(nil).Once()
mockExec.On("Run", tmpDir, "git", []string{"branch", "-D", "feature-branch"}).Return(nil).Once()

return AppInterface{
Expand All @@ -424,7 +405,6 @@ func TestUpdatePackageTag(t *testing.T) {

_ = os.WriteFile(saasFile, []byte("tag: old123"), 0o400)

mockExec.On("Run", tmpDir, "git", []string{"checkout", "master"}).Return(nil).Once()
mockExec.On("Run", tmpDir, "git", []string{"branch", "-D", "feature-branch"}).Return(nil).Once()

return AppInterface{
Expand Down Expand Up @@ -496,7 +476,6 @@ resourceTemplates:
}

mock_exec := new(MockExec)
mock_exec.On("Run", "/path/to/git/dir", "git", []string{"checkout", "master"}).Return(nil).Once()
mock_exec.On("Run", "/path/to/git/dir", "git", []string{"branch", "-D", "feature-branch"}).Return(nil).Once()
mock_exec.On("Run", "/path/to/git/dir", "git", []string{"checkout", "-b", "feature-branch", "master"}).Return(nil).Once()

Expand Down Expand Up @@ -531,7 +510,8 @@ resourceTemplates:
}

mock_exec := new(MockExec)
mock_exec.On("Run", "/path/to/git/dir", "git", []string{"checkout", "master"}).Return(errors.New("failed to checkout master branch")).Once()
mock_exec.On("Run", "/path/to/git/dir", "git", []string{"branch", "-D", "feature-error"}).Return(nil).Once()
mock_exec.On("Run", "/path/to/git/dir", "git", []string{"checkout", "-b", "feature-error", "master"}).Return(errors.New("failed to create branch")).Once()

return AppInterface{
GitDirectory: "/path/to/git/dir",
Expand All @@ -542,16 +522,17 @@ resourceTemplates:
current_git_hash: "currentGitHash",
promotion_git_hash: "promotionGitHash",
branch_name: "feature-error",
expected_err: "failed to checkout master branch",
expected_err: "failed to create branch",
verify: func(t *testing.T, _ string) {},
},
"fails_when_git_directory_does_not_exist": {
setup: func(t *testing.T) (AppInterface, string) {
non_existent_dir := filepath.Join(os.TempDir(), "nonexistent-dir")
mock_exec := new(MockExec)
// Simulate failure on git checkout due to missing directory
mock_exec.On("Run", non_existent_dir, "git", []string{"checkout", "master"}).
Return(errors.New("failed to checkout master branch")).Once()
mock_exec.On("Run", non_existent_dir, "git", []string{"branch", "-D", "feature-no-dir"}).Return(nil).Once()
mock_exec.On("Run", non_existent_dir, "git", []string{"checkout", "-b", "feature-no-dir", "master"}).
Return(errors.New("failed to create branch")).Once()

return AppInterface{
GitDirectory: non_existent_dir,
Expand All @@ -562,7 +543,7 @@ resourceTemplates:
current_git_hash: "abcdef",
promotion_git_hash: "123456",
branch_name: "feature-no-dir",
expected_err: "failed to checkout master branch",
expected_err: "failed to create branch",
verify: func(t *testing.T, _ string) {},
},
"fails_when_file_does_not_exist": {
Expand All @@ -571,7 +552,6 @@ resourceTemplates:
saasFilePath := filepath.Join(tmpDir, "nonexistent.yaml") // file intentionally does not exist

mockExec := new(MockExec)
mockExec.On("Run", tmpDir, "git", []string{"checkout", "master"}).Return(nil).Once()
mockExec.On("Run", tmpDir, "git", []string{"branch", "-D", "feature-fail-read"}).Return(nil).Once()
mockExec.On("Run", tmpDir, "git", []string{"checkout", "-b", "feature-fail-read", "master"}).Return(nil).Once()

Expand Down
14 changes: 14 additions & 0 deletions cmd/promote/pko/pko.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,20 @@ func (p pkoOptions) ValidatePKOOptions() error {
}

func PromotePackage(appInterface git.AppInterface, serviceName string, packageTag string, hcp bool) error {
// Fetch and sync with origin/master FIRST before reading any files
// to ensure we're working with the latest app-interface state
fmt.Println("Syncing app-interface with origin/master...")
if err := appInterface.GitExecutor.Run(appInterface.GitDirectory, "git", "fetch", "origin", "master"); err != nil {
Copy link
Contributor

@joshbranham joshbranham Feb 11, 2026

Choose a reason for hiding this comment

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

To your point in the PR, what happens if someone's default remote is not "origin"?

return fmt.Errorf("failed to fetch origin/master: %v", err)
}
if err := appInterface.GitExecutor.Run(appInterface.GitDirectory, "git", "checkout", "master"); err != nil {
return fmt.Errorf("failed to checkout master: %v", err)
}
if err := appInterface.GitExecutor.Run(appInterface.GitDirectory, "git", "reset", "--hard", "origin/master"); err != nil {
Copy link
Contributor

@joshbranham joshbranham Feb 11, 2026

Choose a reason for hiding this comment

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

This could cause unintended consequences if someone has something committed locally.

return fmt.Errorf("failed to sync master with origin/master: %v", err)
}
fmt.Println("Successfully synced local master with origin/master")

services, err := saas.GetServiceNames(appInterface, saas.OSDSaasDir, saas.BPSaasDir, saas.CADSaasDir)
if err != nil {
return err
Expand Down
14 changes: 14 additions & 0 deletions cmd/promote/saas/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,20 @@ func generateTestLogsURL(appInterface git.AppInterface, operatorName, gitHash st
}

func servicePromotion(appInterface git.AppInterface, serviceName, gitHash string, namespaceRef string, osd, hcp, hotfix bool) error {
// Fetch and sync with origin/master FIRST before reading any files
// to ensure we're working with the latest app-interface state
fmt.Println("Syncing app-interface with origin/master...")
if err := appInterface.GitExecutor.Run(appInterface.GitDirectory, "git", "fetch", "origin", "master"); err != nil {
return fmt.Errorf("failed to fetch origin/master: %v", err)
}
if err := appInterface.GitExecutor.Run(appInterface.GitDirectory, "git", "checkout", "master"); err != nil {
return fmt.Errorf("failed to checkout master: %v", err)
}
if err := appInterface.GitExecutor.Run(appInterface.GitDirectory, "git", "reset", "--hard", "origin/master"); err != nil {
return fmt.Errorf("failed to sync master with origin/master: %v", err)
}
fmt.Println("Successfully synced local master with origin/master")

_, err := GetServiceNames(appInterface, OSDSaasDir, BPSaasDir, CADSaasDir)
if err != nil {
return err
Expand Down
5 changes: 5 additions & 0 deletions cmd/promote/saas/utils_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -531,3 +531,8 @@ func TestHotfixValidation(t *testing.T) {
})
}
}

// Note: The origin/master sync logic added to servicePromotion is tested indirectly
// through the git package tests (cmd/promote/git/app_interface_test.go).
// The sync uses git.AppInterface.GitExecutor.Run() which is thoroughly tested there.
// Full integration testing of the sync behavior should be done through end-to-end tests.