Need to handle error when writing /etc/hosts #170
Conversation
|
Hey mattcui! Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA. |
|
We have created an issue in Pivotal Tracker to manage this: https://www.pivotaltracker.com/story/show/133721557 The labels on this github issue will be updated when the story is started. |
|
@maximilien The code is ready for review. Thanks. |
|
@mattcui please address each comment. I cannot review if you just ignore comments. What's the point? |
| vm, err := creator.Create(agentID, stemcell, cloudProps, networks, env) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| Expect(vm.ID()).To(Equal(1234567)) | ||
| }) | ||
| It("returns error when creating a new SoftLayerVM without bosh ip using non-root user", func() { |
There was a problem hiding this comment.
I think this It description is incorrect
| @@ -154,7 +154,7 @@ func UpdateEtcHostsOfBoshInit(record string) (err error) { | |||
| logger := boshlog.NewWriterLogger(boshlog.LevelError, os.Stderr, os.Stderr) | |||
| fs := boshsys.NewOsFileSystem(logger) | |||
|
|
|||
| err = fs.WriteFile("/etc/hosts", buffer.Bytes()) | |||
| err = fs.WriteFile(fileName, buffer.Bytes()) | |||
There was a problem hiding this comment.
You might want to mv file instead of writing. So write to a temp file and mv to the /etc/hosts this is because mv operation is atomic but cp is not. This matters if there is an error during cp. For mv you will either have success or failure but not a file that is corrupted or half written.
| LengthOfHostName int | ||
| TIMEOUT time.Duration | ||
| POLLING_INTERVAL time.Duration | ||
| LocalDiskFlagNotSet bool |
There was a problem hiding this comment.
I am uncomfortable why these globals exist. I can understand if they need to be set. But the LocalDNSConfigurationFile is always /etc/hosts why not make it a constant? Is there a case where it will be different?
|
@mattcui see my submitted reviews. Please address each so we can move further. Thanks. |
|
@mattcui still waiting for changes on this one that addresses my comments. |
cf-gitbot
added
in progress
delivered
accepted
and removed
unscheduled
in progress
delivered
labels
|
Cannot merge. You need to rebase and resolve conflicts |
|
Still have conflicts |
|
@maximilien I solved all conflicts, it's ready for review and merge now, thanks. |
To address #133650831, the problem is reported from issue #168