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

Revert "refactor(jenkins/pipelines,pipelines): use the new dockerfile url location" #2959

Merged
merged 1 commit into from
May 11, 2024

Conversation

wuhuizuo
Copy link
Collaborator

@wuhuizuo wuhuizuo commented May 11, 2024

Reverts #2958

It broken the dev-builds since of the issue, we must solve it atomic:

[2024-05-10T10:29:43.850Z] + docker build --pull -t 325168c9-8528-454b-9e0f-2041080ee440 . --build-arg 'BASE_IMG='

Copy link

ti-chi-bot bot commented May 11, 2024

I have already done a preliminary review for you, and I hope to help you do a better job.

Based on the pull request title and description, it seems that the changes made in #2958 are being reverted. The diff shows that the Dockerfile URLs are being changed from the new location to the old one.

Potential problems with this pull request could be:

  • It is not clear why the changes made in refactor(jenkins/pipelines,pipelines): use the new dockerfile url location #2958 are being reverted, and if it is the right course of action.
  • Reverting changes can sometimes lead to issues, if there were dependencies on the changes made in the original pull request.
  • The URLs for the Dockerfiles are being changed back to the old location, which may not be the best course of action if the new location was better for some reason.

Some suggestions to fix these problems could be:

  • The pull request should have more context to explain the reasons for the revert, and if it is the correct decision to make.
  • It would be better to investigate if there were any dependencies on the changes made in refactor(jenkins/pipelines,pipelines): use the new dockerfile url location #2958, and if there are, address them accordingly.
  • If the new location for the Dockerfiles was better, it would be better to find a way to fix the issues with that location, rather than reverting back to the old location.

@ti-chi-bot ti-chi-bot bot added the size/S label May 11, 2024
@wuhuizuo
Copy link
Collaborator Author

/review default

Copy link

ti-chi-bot bot commented May 11, 2024

@wuhuizuo:

I have already done a preliminary review for you, and I hope to help you do a better job.

It seems this pull request is reverting a previous change that broke the dev-builds due to a problem with the Docker build command. The change that is being reverted is a refactor of the Jenkins pipelines that changed the location of the Dockerfile URLs.

There are no potential problems with the code changes since the pull request is just reverting a previous change. However, there might be issues with the dev builds if the previous problem is not resolved.

My suggestion is to investigate the issue with the Docker build command and fix it before attempting to refactor the Jenkins pipelines. Once the issue is fixed, the previous refactor can be revisited and implemented again.

In response to this:

/review default

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@wuhuizuo wuhuizuo added the lgtm label May 11, 2024
@wuhuizuo
Copy link
Collaborator Author

/approve

Copy link

ti-chi-bot bot commented May 11, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wuhuizuo

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

The pull request process is described 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

@ti-chi-bot ti-chi-bot bot added the approved label May 11, 2024
@wuhuizuo wuhuizuo merged commit 303ece9 into main May 11, 2024
0 of 2 checks passed
@wuhuizuo wuhuizuo deleted the revert-2958-fix/renew-dockerfile-location branch May 11, 2024 01:01
ti-chi-bot bot pushed a commit that referenced this pull request May 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

1 participant