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

Remove experimental warning on audit mode. #3299

Merged

Conversation

juliandunn
Copy link
Contributor

I'm going to suggest we remove this to avoid freaking out customers since we have been shipping this mode for some time.

/cc @chef/analytics-data-integrations

@jaym
Copy link
Contributor

jaym commented Apr 29, 2015

Did we solve the issue around audit mode mangling exceptions?

@juliandunn
Copy link
Contributor Author

What do you mean by "mangling exceptions"?

@jaym
Copy link
Contributor

jaym commented Apr 29, 2015

625373d#diff-c34d08382bb25f61e44ecbd5f50f4166
This was one of the regressions in 12.2 and we ended up patching it. By making audit mode the default, the people we broke before will break again.

@jaym
Copy link
Contributor

jaym commented Apr 29, 2015

I think the example we got was SystemExit and how this broke people's CI. Not sure if there is anything else that we need to consider.
cc @tyler-ball

@juliandunn
Copy link
Contributor Author

My PR doesn't make audit mode the default; I don't know that we need to do that right now. I'm just removing warnings here.

@jaym
Copy link
Contributor

jaym commented Apr 29, 2015

My bad, I guess this is harmless then.

@tyler-ball
Copy link
Contributor

@jdmundrawala If audit mode is disabled (the default) we don't wrap any raised exceptions, we just re-raise them - #3090

@tyler-ball
Copy link
Contributor

👍 On this change. I could see people being concerned this message gets changed in a patch version. Can we hold off on it until the next major version (12.4)?

@juliandunn
Copy link
Contributor Author

@tyler-ball Sure, that's fine

@juliandunn
Copy link
Contributor Author

Per @tyler-ball's message -- looks like we're on the cusp of releasing 12.4. Can I get this warning removal in there? /cc @chef/client-maintainers

@tyler-ball tyler-ball added this to the 12.4.0 milestone Jun 2, 2015
@danielsdeleo
Copy link
Contributor

👍 as long as we're sufficiently confident that the design is fairly stable and we don't need to change it in incompatible ways in the future.

@jtimberman
Copy link
Contributor

The main thing off the top of my head is this

#3482

It's a bug fix but if people did the should syntax in their tests that change would break them.

@juliandunn
Copy link
Contributor Author

@jtimberman I'm going to argue that whether that's a bug or not doesn't generally change the shape of the design of audit mode, and at this point we've got enough usage & published enough articles about it that we should just say it's shipped.

@danielsdeleo
Copy link
Contributor

Well then you shouldn't break any working code. OTOH, that may be a moot point: #3482 (comment)

thommay added a commit that referenced this pull request Jun 9, 2015
…al-warning

Remove experimental warning on audit mode.
@thommay thommay merged commit 1d42bcc into chef:master Jun 9, 2015
@chef chef locked and limited conversation to collaborators Nov 16, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants