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

Add make targets to generate release Kubernetes manifests #613

Merged
merged 1 commit into from
May 3, 2022

Conversation

abhinavmpandey08
Copy link
Contributor

Description

This PR adds Makefile target release to generate Kubernetes manifests for tink-server and tink-controller

Why is this needed

This is needed to generate the release manifests using kustomize

How Has This Been Tested?

Ran make release and verified that the manifest gets generated properly

How are existing users impacted? What migration steps/scripts do we need?

No user impact

Checklist:

I have:

  • updated the documentation and/or roadmap (if required)
  • added unit or e2e tests
  • provided instructions on how to upgrade

@abhinavmpandey08 abhinavmpandey08 force-pushed the generate-manifests branch 3 times, most recently from 5c8bc11 to 6e47879 Compare May 3, 2022 01:18
@codecov
Copy link

codecov bot commented May 3, 2022

Codecov Report

Merging #613 (3e1699e) into main (cd86d68) will not change coverage.
The diff coverage is n/a.

❗ Current head 3e1699e differs from pull request most recent head a1906ff. Consider uploading reports for the commit a1906ff to get more accurate results

@@           Coverage Diff           @@
##             main     #613   +/-   ##
=======================================
  Coverage   44.37%   44.37%           
=======================================
  Files          61       61           
  Lines        3491     3491           
=======================================
  Hits         1549     1549           
  Misses       1858     1858           
  Partials       84       84           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cd86d68...a1906ff. Read the comment docs.

jacobweinstock
jacobweinstock previously approved these changes May 3, 2022
Copy link
Member

@jacobweinstock jacobweinstock left a comment

Choose a reason for hiding this comment

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

lgtm

kube.mk Outdated Show resolved Hide resolved
kube.mk Outdated
$(RELEASE_DIR):
mkdir -p $(RELEASE_DIR)/

REGISTRY ?= ghcr.io
Copy link
Member

Choose a reason for hiding this comment

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

should this be quay.io/tinkerbell?

…rver and tink-controller

Signed-off-by: Abhinav Pandey <abhinavmpandey08@gmail.com>
@jacobweinstock jacobweinstock added the ready-to-merge Signal to Mergify to merge the PR. label May 3, 2022
@mergify mergify bot merged commit ab7fdff into tinkerbell:main May 3, 2022

.PHONY: release
release: clean-release
$(MAKE) set-manager-manifest-image MANIFEST_IMG=$(REGISTRY)/$(TINK_SERVER_IMAGE_NAME) MANIFEST_TAG=$(TINK_CONTROLLER_IMAGE_TAG)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @abhinavmpandey08 was this supposed to be TINK_CONTROLLER_IMAGE_TAG?

@mmlb mmlb mentioned this pull request May 3, 2022
mergify bot added a commit that referenced this pull request May 3, 2022
## Description

#613 introduced a few unwanted bits and a typo.

## Why is this needed

#613 did not use tools.go setup we have for using pinned go tools.

I reworked it so it also didn't do recursive make as that is considered harmful and setup better dependencies so that we let make be better equipped to do the right thing. This way we avoid using make as a dumb command runner, which if not done can lead to racy/broken makefiles (see hook for example).

I fixed a typo that would cause the manager to use tink-server image, likely to not work :D.

In the process I noticed that we don't have `generate-manifests` hooked up to be run as part of ci to detect drift, so I added it as a dependency of `generated` as it should have been from the beginning.

## How Has This Been Tested?

I ran make a bunch.
@displague displague added this to the 0.7.0 milestone Aug 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge Signal to Mergify to merge the PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants