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

🐛 Fix robot log and switch to pflag #991

Merged
merged 1 commit into from
Sep 30, 2023
Merged

🐛 Fix robot log and switch to pflag #991

merged 1 commit into from
Sep 30, 2023

Conversation

janiskemper
Copy link
Contributor

What this PR does / why we need it:
Fix logger of robot api and switch to pflag package

TODOs:

  • squash commits
  • include documentation
  • add unit tests

Tiltfile Outdated Show resolved Hide resolved
@janiskemper janiskemper marked this pull request as ready for review September 29, 2023 13:41
@syself-bot syself-bot bot added the area/code Changes made in the code directory label Sep 29, 2023
Fix logger of robot api and switch to pflag package
@syself-bot syself-bot bot added the size/S Denotes a PR that changes 20-50 lines, ignoring generated files. label Sep 29, 2023
@batistein batistein merged commit be18ae9 into main Sep 30, 2023
9 checks passed
@batistein batistein deleted the fix-lint branch September 30, 2023 07:57
@apricote
Copy link
Contributor

apricote commented Oct 5, 2023

Just FYI, this caused my existing deployment to fail after updating the image because one flag i was using had a single hyphen (-metrics-bind-address) and this was no longer allowed.


OT: It would be very nice if CAPH supported the same flags as other CAPI components, to make using it with the capi-operator nicer. In this case, the capi-operator expects the flag --metrics-bind-addr instead of CAPHs --metrics-bind-address. Because of this I need to add the flag manually instead of using the field provided in the InfrastructureManager CR.

I will open an issue for this when I find the time.

@janiskemper
Copy link
Contributor Author

then you were smarter than us, because we realized that our example deployments did it wrongly and the flags did not take effect (they were not changing anything really, that's why we also didn't realize).

I guess your proposed change makes sense. AFAIK we should be mostly aligned with other providers though.

@batistein can we document this change somewhere in case other people run into this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/code Changes made in the code directory size/S Denotes a PR that changes 20-50 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants