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

[fixed] : Order of Local Source between Builds and BuildRuns #1507

Closed
wants to merge 4 commits into from
Closed

[fixed] : Order of Local Source between Builds and BuildRuns #1507

wants to merge 4 commits into from

Conversation

Adarsh-jaiss
Copy link
Member

@Adarsh-jaiss Adarsh-jaiss commented Feb 27, 2024

Changes

This Pull Request fixes #1500 by improving the "isLocalCopyBuildSource" function by reordering the conditionals to prioritize the local source from the BuildRun over that of the Build. Here's a summary of the changes made:

  • The check for the local source in the BuildRun is moved to the top of the function. This adjustment ensures that if a local source is defined in the BuildRun, it will be returned without further checks.
  • If no local source is found in the BuildRun, the function then checks if the Build has a local source defined and returns it if found.
  • If neither the BuildRun nor the Build has a local source defined, the function returns nil.
    Removed redundant condition:

With the first conditional handling the case where a local source is defined in the BuildRun, there's no need to check if the Build has a local source if the BuildRun already has one.

Submitter Checklist

  • Includes tests if functionality changed/was added
  • Includes docs if changes are user-facing
  • Set a kind label on this PR
  • Release notes block has been filled in, or marked NONE

See the contributor guide
for details on coding conventions, github and prow interactions, and the code review process.

Release Notes

@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Feb 27, 2024
@openshift-ci openshift-ci bot added the release-note Label for when a PR has specified a release note label Feb 27, 2024
Copy link
Contributor

openshift-ci bot commented Feb 27, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign otaviof for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@Adarsh-jaiss Adarsh-jaiss changed the title Build bugs [fixed] : Order of Local Source between Builds and BuildRuns Feb 27, 2024
Comment on lines 32 to 33
// Note: In v1alpha1 we will append all sources across builds and buildruns, and then return
// the first source of type Local.
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment becomes an orphan by your change, nonetheless we think this comment can be removed anyway. Could you amend that in your change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah sure @HeavyWombat . will do it ASAP!

Copy link
Member

@SaschaSchwarze0 SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

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

Can you please rebase your branch. Commit "Fixed buildCredentialserences function name" was already merged in #1506.

Also, please check the release notes section of your PR description. You need to remove the HTML comment (<!-- -->) and select one section like I had done it for you in your previous PR.

@pull-request-size pull-request-size bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 29, 2024
@openshift-ci openshift-ci bot added release-note-none Label for when a PR does not need a release note release-note Label for when a PR has specified a release note do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. and removed release-note Label for when a PR has specified a release note release-note-none Label for when a PR does not need a release note labels Feb 29, 2024
@Adarsh-jaiss
Copy link
Member Author

I think i have messed up a lot of things. I'll just create a new PR

@openshift-ci openshift-ci bot added release-note Label for when a PR has specified a release note and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note Label for when a PR has specified a release note size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[BUG] Order of Local Source between Builds and BuildRuns
3 participants