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

Refactor check_build_dependencies #35742

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

Charl1996
Copy link
Contributor

Technical Summary

A refactor based on this comment.

Safety Assurance

Tested locally

Safety story

Small change which only affects how a previous app build is fetched.

QA Plan

No QA needed

Rollback instructions

  • This PR can be reverted after deploy with no further considerations

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@Charl1996 Charl1996 added the product/invisible Change has no end-user visible impact label Feb 6, 2025
@Charl1996 Charl1996 requested a review from mkangia February 6, 2025 07:22
descending=True,
limit=1,
skip=1,
).first()
Copy link
Contributor

Choose a reason for hiding this comment

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

just confirming that this returns None in case of no previous build? And you tested that I believe for a fresh app then?

Copy link
Contributor

Choose a reason for hiding this comment

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

Additionally, can we further improve this by using

reduce=False,
include_docs=False

I believe the key itself would have the ID of the previous build, so you don't need the doc.
Or if works you can wrap the doc returned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just confirming that this returns None in case of no previous build?

Yes, it returns None.

Additionally, can we further improve this by using

The include_docs key has a default of False, but not sure about reduce. Will update.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh feel free to check what it does, its been a while for me and I remember that these both were to be used together.

Copy link
Contributor

Choose a reason for hiding this comment

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

include_docs key has a default of False

Oh cool, does this view then return a key? and not a doc? I was confused by the naming of previous_build_doc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product/invisible Change has no end-user visible impact
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants