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

docs: added BootstrapConfig for Merge Option #5259

Merged
merged 1 commit into from
Feb 13, 2025

Conversation

Anu-Ra-g
Copy link
Contributor

@Anu-Ra-g Anu-Ra-g commented Feb 11, 2025

What type of PR is this?
Documentation for Merge Option in Custom bootstrap for EnvoyProxy

What this PR does / why we need it:
This PR contains the requested documentation updates for the custom bootstrap config for envoyproxy along with the Merge option

Which issue(s) this PR fixes:
Fixes #2545

Release Notes: No

Note: Had to previous PR due to some issues.

@Anu-Ra-g Anu-Ra-g marked this pull request as ready for review February 11, 2025 16:41
@Anu-Ra-g Anu-Ra-g requested a review from a team as a code owner February 11, 2025 16:41
@arkodg
Copy link
Contributor

arkodg commented Feb 11, 2025

thanks, have you tested that the config works ?
also why are all the test helm files deleted ?

Copy link

codecov bot commented Feb 11, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.93%. Comparing base (6022624) to head (d375b80).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5259      +/-   ##
==========================================
- Coverage   67.94%   67.93%   -0.01%     
==========================================
  Files         214      214              
  Lines       33522    33522              
==========================================
- Hits        22776    22773       -3     
- Misses       9358     9360       +2     
- Partials     1388     1389       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Anu-Ra-g
Copy link
Contributor Author

@arkodg The configs are working fine. I pushed the helm charts in this PR but it's not appearing here. I'm looking into it.

@Anu-Ra-g
Copy link
Contributor Author

Anu-Ra-g commented Feb 11, 2025

@arkodg The helm charts are getting deleted when I run make generate. I pushed the updates after running make generate. Can you please help me out here?

Copy link
Member

Choose a reason for hiding this comment

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

why remove this?

@zirain
Copy link
Member

zirain commented Feb 11, 2025

  1. run make -k gen-check to make sure everything is generated
  2. do not recreate PRs, EG use squash merge mode for git.

@Anu-Ra-g
Copy link
Contributor Author

@zirain when I run the make -k gen-check, the helm charts are getting deleted, even without any changes to the branch

This dependency helm-generate, is causing the issue

.PHONY: gen-check
gen-check: format generate manifests protos go.testdata.complete
	@$(LOG_TARGET)
	@if [ ! -z "`git status --porcelain`" ]; then \
		$(call errorlog, ERROR: Some files need to be updated, please run 'make generate', 'make manifests' and 'make protos' to include any changed files to your PR); \
		git diff --exit-code; \
	fi
.PHONY: generate
generate: ## Generate go code from templates and tags
generate: kube-generate docs-api helm-generate go.generate copy-current-release-docs kube-generate-examples
.PHONY: helm-generate
helm-generate:
	@for chart in $(CHARTS); do \
  		$(LOG_TARGET); \
  		$(MAKE) $(addprefix helm-generate., $$(basename $${chart})); \
  	done

@zirain
Copy link
Member

zirain commented Feb 12, 2025

AFAIK, it won't cause any unexcepted issues.

@Anu-Ra-g
Copy link
Contributor Author

@zirain I cloned the repo again in a new folder and ran make -k gen-check, like you said. The result was still the same. The helm charts are getting removed.

@zirain
Copy link
Member

zirain commented Feb 12, 2025

@zirain I cloned the repo again in a new folder and ran make -k gen-check, like you said. The result was still the same. The helm charts are getting removed.

you must did something wrong, that why gen-check CI failed.

@Anu-Ra-g
Copy link
Contributor Author

@zirain My environment is set to go version 1.23.6. That might be a problem?

@Anu-Ra-g Anu-Ra-g closed this Feb 12, 2025
@Anu-Ra-g Anu-Ra-g reopened this Feb 12, 2025
@Anu-Ra-g
Copy link
Contributor Author

Anu-Ra-g commented Feb 13, 2025

Thanks for the help @zirain @arkodg

Copy link
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

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

LGTM thanks !

@arkodg arkodg requested a review from zirain February 13, 2025 16:48
@zirain zirain merged commit a38846d into envoyproxy:main Feb 13, 2025
28 checks passed
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.

docs: Add a section for adding a Custom bootstrap using the Merge Option
3 participants