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

Support a way to skip implied get after put. #8492

Merged
merged 1 commit into from Oct 18, 2022

Conversation

evanchaoli
Copy link
Contributor

@evanchaoli evanchaoli commented Jul 22, 2022

Changes proposed by this PR

closes #3299

  • done

Notes to reviewer

There have been much such requests, and I fully understood why implied get is needed in order to keep a resource fresh.

But we may also honor reality. There are a lot of put only resources running in pipelines, mostly about notifications, like put: email, put: slack, etc. Implied get just run an empty container, and plug nested image-check + image-get. We have noticed that Intermittently implied get happened to be placed on a busy worker thus stuck for some time, but they didn't do anything, which hurts stability of build execution.

Adding such an option to opt-out implied get will

  • eliminate unnecessary failure points, thus improve build stability
  • reduce build execute time
  • save worker resources

The other reason I reopen the issue is that, Concourse proposed other approaches to solve the problem, but now those approaches seem to be no ETA anymore.

If you agree to merge this PR, I can update docs as well.

Release Note

  • Added no_get option to put step to skip implied get. For example:
    - put: email
      no_get: true
      params:
        ...

@evanchaoli evanchaoli requested a review from a team as a code owner July 22, 2022 03:50
@evanchaoli evanchaoli added this to the v7.9.0 milestone Jul 22, 2022
@navdeep-pama navdeep-pama removed this from the v7.9.0 milestone Jul 22, 2022
@simonjohansson
Copy link
Contributor

This is such a simple fix to an annoying thing when you don't need a get for put only resources... :)

Please merge!

@xtremerui xtremerui added this to the v7.9.0 milestone Aug 31, 2022
@simonjohansson
Copy link
Contributor

Thx @xtremerui :)

Signed-off-by: Evan <chaol@vmware.com>
@evanchaoli
Copy link
Contributor Author

@xtremerui I just rebased the code, hopefully tests would pass.

Copy link
Contributor

@xtremerui xtremerui left a comment

Choose a reason for hiding this comment

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

LGTM!

@evanchaoli
Copy link
Contributor Author

@xtremerui As you have approved this PR, please go ahead merge it. I am not going to apply a patch of this PR to our prod.

@xtremerui xtremerui merged commit 821ac8e into concourse:master Oct 18, 2022
@xtremerui
Copy link
Contributor

@evanchaoli please create a PR to update the doc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add an option to skip implied get of put
4 participants