Skip to content

Explicitly disallow overwriting in some cases#12

Closed
just-be-dev wants to merge 1 commit into
masterfrom
no-update-assert
Closed

Explicitly disallow overwriting in some cases#12
just-be-dev wants to merge 1 commit into
masterfrom
no-update-assert

Conversation

@just-be-dev

@just-be-dev just-be-dev commented Dec 14, 2022

Copy link
Copy Markdown

In oxidecomputer/omicron#885 we explicitly opted not to use the assert contents method because we didn't want folks easily updating the file. While that's fine on the face of it, the error experience got objectively worse and has cost a non-trivial amount of debugging time. I'd put in a change to make the error message more friendly, but it broke half the assumptions of the test.

This PR sketches out a possible option where we have a method that explicitly doesn't perform overwriting. This is just a rough PoC, hoping to more generate conversation and figure out a direction.

@just-be-dev just-be-dev changed the title Enable disallowing overwrite Explicitly disallow overwriting in some cases Dec 14, 2022
@ahl

ahl commented Dec 15, 2022

Copy link
Copy Markdown
Collaborator

Is the thing you miss in omicron that expectorate shows you the diff between actual and expected? What about using something like https://crates.io/crates/pretty_assertions

If we do want to add something to expectorate for this case, I think we'd want the following properties:

  1. Help the person creating the initial file (i.e. comparison against a non-existent file would be a special case)
  2. Don't include text about overriding the contents (since that doesn't apply)
  3. Do require the caller to include specific text to include in the case of a mismatch to guide the engineer experiencing the failure regarding what to do next

Rather than a long name, perhaps something more definitive like confirm_contents() or require_contents.

Let me know if you think we should proceed or if pretty_assertions (or something like it) would be fine.

@ahl ahl closed this Jun 20, 2023
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.

2 participants