9297 feature flags #9299
9297 feature flags #9299
Conversation
poikilotherm
added
Component: Code Infrastructure
poikilotherm
added
User Role: Sysadmin
| Adding a Feature Flag | ||
| ^^^^^^^^^^^^^^^^^^^^^ | ||
|
|
||
| Some parts of our codebase might be opt-in only. Experimental or optional feature previews can be switched on using our |
There was a problem hiding this comment.
Suggested addition after the first sentence. They may be only for developers/not intended for production, and/or temporary. (Experimental or optional features that can be used in production and are intended to remain, perhaps with some change over time, Experimental or optional feature previews...
There was a problem hiding this comment.
Actually I think I got what causes the confusion here now - we already do experimental things and simply used configuration and not flags for this before! Are you talking about "dev only" because of this? (That's actually not what I see in this mechanism)
Here's a bold statement: as we seem to getting more and more of these now, how about we make all the other experimental things using this flag thing, too, so we are consistent again?
There was a problem hiding this comment.
That would put #9290 which shouldn't get used in production and presumably will be going away back in the list of things you can use now and that we expect to stay around. I suppose that could be handled by strong messaging on that PR.
It also means anyone using these features has to reconfigure for 5.13? And we'd have to decide what to do with features that require some config info - do all their settings move to be under feature or do you turn them on one way and configure them elsewhere? Again, solvable problems, but I don't see a lot of benefit either.
This is definitely something where there probably isn't one right answer and I don't care much how it comes out as long as we don't confuse experimental things you can use in production with things that shouldn't through some form of messaging.
There was a problem hiding this comment.
That would put #9290 which shouldn't get used in production and presumably will be going away back in the list of things you can use now and that we expect to stay around. I suppose that could be handled by strong messaging on that PR.
That is a good and valid point - maybe there should be a forth column in the list that indicates security level? Maybe this should even be marked @Deprecated in code if it is serious for production so it becomes more visible this needs more work.
It also means anyone using these features has to reconfigure for 5.13? And we'd have to decide what to do with features that require some config info - do all their settings move to be under feature or do you turn them on one way and configure them elsewhere? Again, solvable problems, but I don't see a lot of benefit either.
Well if we want to migrate existing things to this mechanism, we certainly will be able to make this backward compatible. (As done before for JvmSettings)
This is definitely something where there probably isn't one right answer and I don't care much how it comes out as long as we don't confuse experimental things you can use in production with things that shouldn't through some form of messaging.
I agree. Another thought: while the session auth thing is maybe not perfectly secure, isn't this something that we probably will turn on during the process of moving to the SPA at least at demo.dataverse.org? We will need that session auth thing if we want to demonstrate using both in parallel. This kind of makes this thing a bit blury.
There was a problem hiding this comment.
I think the existing line conveys the main points:
- opt-in
- experimental
- optional
That said, no objection if someone wants to add more. To me, feature flags are a common term in the industry. Most developers have probably heard of them.
|
sizing:
|
There was a problem hiding this comment.
I just looked at the code and the following docs:
- https://dataverse-guide--9299.org.readthedocs.build/en/9299/developers/configuration.html#adding-a-feature-flag
- https://dataverse-guide--9299.org.readthedocs.build/en/9299/installation/config.html#feature-flags
It all looks good to me. Can we move this to QA?
|
@pdurbin any thoughts about renaming? Or making the scope smaller? Or migrating other experimental things to use this? (Could create a follow-up issue for this.) |
mreekie
added this to ▶ SPRINT READY
in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34)
via automation
kcondon
moved this from Ready for QA ⏩
to Review 🔎
in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34)
kcondon
moved this from Review 🔎
to QA ✅
in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34)
kcondon
moved this from QA ✅
to Ready for QA ⏩
in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34)
kcondon
moved this from Ready for QA ⏩
to QA ✅
in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34)
kcondon
moved this from QA ✅
to Ready for Review ⏩
in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34)
kcondon
moved this from Ready for Review ⏩
to IQSS Team - In Progress 💻
in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34)
|
I wasn't aware that this is now a blocker for @GPortas - sorry! What I put in my old head was that you would do sth. to be non-blocked by this. Sorry for the misunderstanding involved! I do have some stuff saved locally as a stash to evolve this into "EmergingFeature" and "ExperimentalFeature", addressing more of Jim's comments and which is an output of a discussion @GPortas and I had on Zoom. I don't recall if we took any notes. FWIW, I'll push this stuff to this branch as is so we can come back to it later if we need to. As @pdurbin is happy with this PR and as it unblocks @GPortas, I'll put this back into review. |
poikilotherm
moved this from IQSS Team - In Progress 💻
to Ready for QA ⏩
in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34)
|
@poikilotherm thanks! ❤️ There are no new commits since I tested this, so I'll just go ahead and move it to QA. |
There was a problem hiding this comment.
I tested this and it seems to work well: #9299 (comment)
Moving to QA.
kcondon
moved this from Ready for QA ⏩
to QA ✅
in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34)
kcondon
moved this from QA ✅
to Ready for QA ⏩
in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34)
|
grooming:
|
kcondon
moved this from Ready for QA ⏩
to QA ✅
in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34)
|
This passes basic smoke test. I did not test the mechanism as there is not yet any example feature using this. I'm relying on Phil's test case results and the existing unit test for targeted testing. Can be merged when integration tests pass. |
|
The API tests never ran. The spin up scripts died here: From https://jenkins.dataverse.org/job/IQSS-Dataverse-Develop-PR/job/PR-9299/8/console |
|
I just merged the latest. Go, API tests! Go! |
|
API tests passed (took about 10 minutes): https://jenkins.dataverse.org/job/IQSS-Dataverse-Develop-PR/job/PR-9299/9/testReport/edu.harvard.iq.dataverse.api/ |
What this PR does / why we need it:
Introduce feature flags to enable/disable experimental and previews of features.
Which issue(s) this PR closes:
Closes #9297
Special notes for your reviewer:
None so far.
Suggestions on how to test this:
Unit test is provided, otherwise nothing fancy to test here...?
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Nope.
Is there a release notes update needed for this change?:
Not sure. Please provide guidance.
Additional documentation:
🔋 included.