Skip to content

Feat/preflight check#334

Closed
fhennig wants to merge 3 commits intomainfrom
feat/preflight-check
Closed

Feat/preflight check#334
fhennig wants to merge 3 commits intomainfrom
feat/preflight-check

Conversation

@fhennig
Copy link
Copy Markdown
Contributor

@fhennig fhennig commented Feb 23, 2023

No description provided.

Comment thread build_product_images.py
Comment on lines +270 to +272
targets = bakefile["group"][product]["targets"]
for t in targets:
tags.extend(bakefile["target"][t]["tags"])
Copy link
Copy Markdown
Contributor

@nightkr nightkr Feb 23, 2023

Choose a reason for hiding this comment

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

product can actually also be a direct target (for example: "I only want to build Hadoop 3.3.4") or a meta-group ("I want to build all versions of all products"). I think a more correct answer would be something like:

Suggested change
targets = bakefile["group"][product]["targets"]
for t in targets:
tags.extend(bakefile["target"][t]["tags"])
for target in bakefile["group"].get(product, {}).get("targets", []):
tags.extend(get_tags_for_product(bakefile, target))
tags.extend(bakefile["target"].get(product, {}).get("tags", [])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It might also be more correct to name the function something like get_tags_for_target.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I see!

Copy link
Copy Markdown
Member

@razvan razvan left a comment

Choose a reason for hiding this comment

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

After trying it locally, I think I would like to have the preflight check a first class citizen step that I can run locally the same way I can build images by calling something like:

$ ./build_product_images.py -i 23.4.0-rc2 -p superset --preflight-check

and having the script actually running preflight the same way it runs docker buildx bake

This would have the advantage that:

  • (obviously) I can easily run the same check on multiple images locally.
  • I can use it with --dry to see what it's doing.
  • You don't need to rely on standard output for json parsing
  • You can get rid of the jq step and give meaningful success (of failure) messages directly from the script.
  • You would take any "business logic" out of the GH workflow file.

You could even create a separate script just for this purpose and /or turn the build_product_images.py script into a python package with multiple entry points.

@razvan
Copy link
Copy Markdown
Member

razvan commented Mar 3, 2023

Alternative PR: #339

@fhennig
Copy link
Copy Markdown
Contributor Author

fhennig commented Mar 6, 2023

superseded by the alternative PR linked above

@fhennig fhennig closed this Mar 6, 2023
@lfrancke lfrancke deleted the feat/preflight-check branch November 1, 2023 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants