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

Restore Force push in autobumper, unify build workflows with new naming convention #12176

Closed

Conversation

KacperMalachowski
Copy link
Contributor

@KacperMalachowski KacperMalachowski commented Oct 18, 2024

Description

Changes proposed in this pull request:

  • Restore Force True in Push Config
  • Fix paths in build workflows for image-autobumper
  • ...

Related issue(s)

@kyma-bot kyma-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 18, 2024
@kyma-bot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@kyma-bot kyma-bot added the cla: yes Indicates the PR's author has signed the CLA. label Oct 18, 2024
@kyma-bot kyma-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 18, 2024
@kyma-bot
Copy link
Contributor

kyma-bot commented Oct 18, 2024

Plan Result

CI link

Only Outputs will be changed.
Change Result (Click me)
Changes to Outputs:
  ~ service_account_keys_cleaner                        = {
      ~ service_account_keys_cleaner_cloud_run_service = {
            id                         = "locations/europe-west4/namespaces/sap-kyma-prow/services/service-account-keys-cleaner"
          ~ metadata                   = [
              ~ {
                  ~ effective_annotations = {
                      ~ "run.googleapis.com/operation-id"   = "b036cedc-cead-452d-bbde-c6e35ba785ba" -> "f4ecd452-abf9-44d6-933c-3931e2a12c01"
                        # (5 unchanged attributes hidden)
                    }
                  ~ generation            = 100 -> 101
                  ~ resource_version      = "AAYkvu/bPi0" -> "AAYkv5QzotE"
                    # (7 unchanged attributes hidden)
                },
            ]
            name                       = "service-account-keys-cleaner"
          ~ status                     = [
              ~ {
                  ~ latest_created_revision_name = "service-account-keys-cleaner-00100-sr6" -> "service-account-keys-cleaner-00101-csx"
                  ~ latest_ready_revision_name   = "service-account-keys-cleaner-00100-sr6" -> "service-account-keys-cleaner-00101-csx"
                  ~ observed_generation          = 100 -> 101
                  ~ traffic                      = [
                      ~ {
                          ~ revision_name   = "service-account-keys-cleaner-00100-sr6" -> "service-account-keys-cleaner-00101-csx"
                            # (4 unchanged attributes hidden)
                        },
                    ]
                    # (2 unchanged attributes hidden)
                },
            ]
            # (6 unchanged attributes hidden)
        }
        # (2 unchanged attributes hidden)
    }
  ~ service_account_keys_rotator                        = {
      ~ service_account_keys_rotator_cloud_run_service   = {
            id                         = "locations/europe-west4/namespaces/sap-kyma-prow/services/service-account-keys-rotator"
          ~ metadata                   = [
              ~ {
                  ~ effective_annotations = {
                      ~ "run.googleapis.com/operation-id"   = "1fec65d8-44ae-422f-aa50-2c34ecbbaa9a" -> "7d7e5aae-43ce-40f0-a112-444354a22d9f"
                        # (5 unchanged attributes hidden)
                    }
                  ~ generation            = 99 -> 100
                  ~ resource_version      = "AAYkvvACsTQ" -> "AAYkv5Q9QpQ"
                    # (7 unchanged attributes hidden)
                },
            ]
            name                       = "service-account-keys-rotator"
          ~ status                     = [
              ~ {
                  ~ latest_created_revision_name = "service-account-keys-rotator-00099-gsr" -> "service-account-keys-rotator-00100-jk6"
                  ~ latest_ready_revision_name   = "service-account-keys-rotator-00099-gsr" -> "service-account-keys-rotator-00100-jk6"
                  ~ observed_generation          = 99 -> 100
                  ~ traffic                      = [
                      ~ {
                          ~ revision_name   = "service-account-keys-rotator-00099-gsr" -> "service-account-keys-rotator-00100-jk6"
                            # (4 unchanged attributes hidden)
                        },
                    ]
                    # (2 unchanged attributes hidden)
                },
            ]
            # (6 unchanged attributes hidden)
        }
        # (3 unchanged attributes hidden)
    }

You can apply this plan to save these new output values to the OpenTofu
state, without changing any real infrastructure.

@kyma-bot kyma-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 18, 2024
@KacperMalachowski KacperMalachowski marked this pull request as ready for review October 18, 2024 12:33
@kyma-bot kyma-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 18, 2024
@KacperMalachowski KacperMalachowski changed the title Fix bumper Restore Force push in autobumper, unify build workflows with new naming convention Oct 18, 2024
akiioto
akiioto previously approved these changes Oct 18, 2024
@kyma-bot kyma-bot added the lgtm Looks good to me! label Oct 18, 2024
@KacperMalachowski
Copy link
Contributor Author

/hold

@kyma-bot kyma-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 18, 2024
@kyma-bot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@KacperMalachowski
Copy link
Contributor Author

/unhold

@kyma-bot kyma-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 18, 2024
Copy link
Contributor

@dekiel dekiel left a comment

Choose a reason for hiding this comment

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

Could you please describe why a force is needed and why you don't want to run test on edited activity?

.github/workflows/build-image-autobumper.yaml Outdated Show resolved Hide resolved
.github/workflows/build-image-autobumper.yaml Outdated Show resolved Hide resolved
@@ -127,13 +131,13 @@ func gitPush(remote, remoteBranch, baseBranch string, repo *git.Repository, auth
}

// Check if the remote head commit is the same as the local head commit
if remoteRef.Hash() == localRef.Hash() {
if remoteRefHash == localRef.Hash() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why you changed that and introduced a variable? Previous version would work fine right? Without variable declaration and without else block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't work. Example: https://github.com/kyma-project/test-infra/actions/runs/11415911461/job/31766505934

With that change I allow to pass the check even if the remote reference doesn't exists as in example.
The whole check is only for preventing metadata-only updates that might cause test retrigger on bump PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add additional condition when checking err value returned from repo.Reference and do not introduce else block and new variable?

@KacperMalachowski
Copy link
Contributor Author

Could you please describe why a force is needed and why you don't want to run test on edited activity?

Force is needed, because we don't want to include previous bumps in the history. The only what we want on bump PR is history from upstream's main and last bump commit.

Edited activity is not connected with code changes (for code the events is called synchronize) and we don't want to retrigger the tests upon PR description changes.

Co-authored-by: Przemek Pokrywka <12400578+dekiel@users.noreply.github.com>
@KacperMalachowski KacperMalachowski marked this pull request as draft October 19, 2024 16:05
@kyma-bot kyma-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 19, 2024
@dekiel
Copy link
Contributor

dekiel commented Oct 21, 2024

Could you please describe why a force is needed and why you don't want to run test on edited activity?

Force is needed, because we don't want to include previous bumps in the history. The only what we want on bump PR is history from upstream's main and last bump commit.

Edited activity is not connected with code changes (for code the events is called synchronize) and we don't want to retrigger the tests upon PR description changes.

The edited action description: "The title or body of a pull request was edited, or the base branch of a pull request was changed."
If you have a PR with tests passed and you change a base branch will this workflow be triggered again for new target branch or not?

@KacperMalachowski
Copy link
Contributor Author

Could you please describe why a force is needed and why you don't want to run test on edited activity?

Force is needed, because we don't want to include previous bumps in the history. The only what we want on bump PR is history from upstream's main and last bump commit.
Edited activity is not connected with code changes (for code the events is called synchronize) and we don't want to retrigger the tests upon PR description changes.

The edited action description: "The title or body of a pull request was edited, or the base branch of a pull request was changed." If you have a PR with tests passed and you change a base branch will this workflow be triggered again for new target branch or not?

It won't, because the head of the PR will be the same

@KacperMalachowski
Copy link
Contributor Author

KacperMalachowski commented Oct 22, 2024

Decided not to continue with current implementation. As go-git has some tricky way to replace git push -f HEAD:remote-branch command. I will research the topic more during implementation of similar tool for one of my side-projects.

I created the follow up issue to implement completly custom bumper: #12153.
Restored original bumper from k8s team with required fixes in: #12184

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add-or-update cla: yes Indicates the PR's author has signed the CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants