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: os.Remove should ignore non existing errors #4502

Merged
merged 2 commits into from
May 4, 2024

Conversation

jippi
Copy link
Contributor

@jippi jippi commented May 3, 2024

what

We've seen cases where atlantis unlock fails, and when inspecting the error from logs, we found that it was caused by Atlantis trying to remove a plan file that does not exist.

This change treats the os.NotExists error as a "success" when removing a file (since the outcome is the same, the file should not be there).

We've seen this in production a few times, but most notably when doing ephemeral testing.

why

Removing a file should not fail if the file does not exist.

@jippi jippi requested review from a team as code owners May 3, 2024 10:24
@jippi jippi requested review from jamengual, lukemassa and nitrocode and removed request for a team May 3, 2024 10:24
@github-actions github-actions bot added the go Pull requests that update Go code label May 3, 2024
@jippi jippi changed the title os.Remove should ignore non existing errors fix: os.Remove should ignore non existing errors May 3, 2024
Copy link
Contributor

@jamengual jamengual left a comment

Choose a reason for hiding this comment

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

LGTM

@jamengual
Copy link
Contributor

@lukemassa any comments? ( since you have been looking at this code recently)

@X-Guardian
Copy link
Contributor

This should at least log a warning within the original functions (not the new utils function), rather than failing silently.

@jippi
Copy link
Contributor Author

jippi commented May 4, 2024

@X-Guardian Happy to add it, but curious why it should log it.

The desired outcome of calling os.Remove() is I want this file not to be there any more which has two happy paths

  1. The file exists, and it was successfully removed
  2. The file did not exist, which is fine, since I didn't want it to be there anyway

Neither of them is a failure or undesired behavior, and you achieved the desired end state of no file should exist in this path - adding a warning to say I wanted to delete a file, but it wasn't there doesn't (in my opinion) add any value, since its not a problem that the file didn't exist in the first place?

@X-Guardian
Copy link
Contributor

To cover scenario 3, where there is a bug in Atlantis and the filename/path has not been built correctly so that is why the delete failed.

Copy link
Member

@GenPage GenPage left a comment

Choose a reason for hiding this comment

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

💯 :shipit:

Straight forward change. Thank you @jippi

@GenPage GenPage merged commit f35b8c6 into runatlantis:main May 4, 2024
24 of 25 checks passed
@GenPage GenPage added bug Something isn't working quick-win Is obviously something Atlantis should do and will take <4 hrs work labels May 4, 2024
@GenPage
Copy link
Member

GenPage commented May 4, 2024

The warning is a good idea. I want to avoid a philosophy of blocking fixes for the sake of perfection. I would like to see a follow-up PR for the warning, as recommended.

@chenrui333 chenrui333 added this to the v0.28.0 milestone May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working go Pull requests that update Go code quick-win Is obviously something Atlantis should do and will take <4 hrs work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants