This repository has been archived by the owner on Sep 24, 2021. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
All the fixes for cluster api v0.1.4 #62
All the fixes for cluster api v0.1.4 #62
Changes from all commits
ac0eeba
414d24e
1448394
c29fe4e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @chuckha,
Why not use
ScanLines
? Even if not, maybe prefer the Golang regex\r?\n
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ScanLines would also work, this was just in my head at the time. If this becomes an issue in the future we can address it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair, but I also don't think there's a good reason for not switching to stdlib.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what you mean,
bytes
is in the stdlibThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, sorry, my response was poorly worded. I was referring to how
ScanLines
uses a more comprehensive matching pattern. If this won't be an issue here because the line endings are guaranteed, then this is fine.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah i see what you mean. Yes, that would definitely be more robust. It will be worth fixing if we need to fix it later as I'm not expecting line endings other than
\n
at this time. The file we're reading is generated by kubeadm on a unix based system.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I know Golang treats
\n
as a special character. If you use it in afmt.Sprintf
(or a variant), Go will always translate\n
to the OS-specific line-ending character(s). I'm not sure if this is the case on scanning though. Either way, as you said, it's a non-issue here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @chuckha,
Should the port always be assumed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, the port should always be assumed as 6443. The apiservers will always be listening on their own IP at port 6443 until we allow users to customize support. This will change when support for that feature is added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
klog.Fatal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, This is covered in #8
I'll time that out soon depending on if the contributor contributes or not