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

[JENKINS-69853] User experimental flags #7299

Merged
merged 8 commits into from
Mar 12, 2023

Conversation

Wadeck
Copy link
Contributor

@Wadeck Wadeck commented Oct 22, 2022

See JENKINS-69853.

This feature proposal adds the possibility to have sort of feature flag per users. The idea behind this is to accelerate the new UI / design work without having to wait for full approval from the community. Publishing something in beta, hidden behind a "user experimental flag" will let the authors to build on top of their previous steps to finalize their feature before the "actual" release.

📷 Screenshots 📷

With some dummy flags

Screenshot_2022-10-23_004541_001


Without any flags

Screenshot_2022-10-23_005114_001


Testing done

Local instance running with some dummy flag classes (like you can see in the screenshot).
Also running the test cases to check the different situations.

Proposed changelog entries

  • Introduce user experimental flags.

Proposed upgrade guidelines

N/A

Submitter checklist

  • The Jira issue, if it exists, is well-described.
  • The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see examples).
    • Fill in the Proposed upgrade guidelines section only if there are breaking changes or changes that may require extra steps from users during upgrade.
  • There is automated testing or an explanation as to why this change has no tests.
  • New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadocs, as appropriate.
  • New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO"), if applicable.
  • [n/a] New or substantially changed JavaScript is not defined inline and does not call eval to ease future introduction of Content Security Policy (CSP) directives (see documentation).
  • [n/a] For dependency updates, there are links to external changelogs and, if possible, full differentials.
  • For new APIs and extension points, there is a link to at least one consumer.
  • ⚠️ No extension points so far, the idea is to let the UX SIG team implementing the flags.

Desired reviewers

@daniel-beck + @basil for the feature flag topic
@timja + @janfaracik + @NotMyFault for the usage of the feature in UI PRs

Maintainer checklist

Before the changes are marked as ready-for-merge:

  • There are at least two (2) approvals for the pull request and no outstanding requests for change.
  • Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
  • Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
  • Proper changelog labels are set so that the changelog can be generated automatically.
  • If the change needs additional upgrade steps from users, the upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).
  • If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

Cool concept


<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:f="/lib/form">
<select class="jenkins-select__input" name="[${it.flagKey}]">
Copy link
Member

Choose a reason for hiding this comment

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

I think https://weekly.ci.jenkins.io/design-library/ToggleSwitch/ would be a more appropriate control

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem with the ToggleSwitch (AFAICT) is that it has only two states.
Having the "default" is important from my PoV so that we can let people decide to "go back" to default.
Huge inspiration from chrome://flags

Screenshot_2022-10-28_181312_001

Copy link
Member

Choose a reason for hiding this comment

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

What is the default state? (i.e. does this need to be shown visually, in the description or should we simplify with just two states)

Copy link
Contributor Author

@Wadeck Wadeck Nov 4, 2022

Choose a reason for hiding this comment

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

The default value in the dropdown is "Default". It means that the user does accept the value provided in the code of the flag (getDefaultValue).

My idea is that we introduce a new feature, we start with disabled by default, so interested users can enable the behavior. During that period, the author can continue improving their feature (without having to wait too much). Then after some months, one can move the feature to be enabled by default. At that moment, the users could decide to rollback by using Disabled.

In parallel, a user who decide that the feature is interesting they can enabled it in advance and when the feature default behavior will be changed, there is no impact for them.

With the different behavior we can see there, we can gather telemetry to determine if the feature is something the community wants.

I want to be able to see data about adoption instead of just having the opinion from loud users (often either very happy or very unhappy).

Copy link
Member

Choose a reason for hiding this comment

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

I mean how does the user know what the default state is? 'Default' doesn't imply what the default actually is.

Copy link
Member

@timja timja Nov 4, 2022

Choose a reason for hiding this comment

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

Yes that looks good

Copy link
Member

@lemeurherve lemeurherve Nov 4, 2022

Choose a reason for hiding this comment

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

Could the "(default)" be added as prefix or suffix of the default entry instead of adding one just for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would say that putting the "help" the closer to where it is used seems better (yeah talking about some pixels :D)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After a quick discussion with Hervé (due to my misunderstanding of his argument), I think I forgot to mention the most important argument about the tristate.
As I would like to introduce telemetry on the flags to ease the data gathering, having the "default" option is a way to have a "no vote" / "blank vote" in terms of adoption of the feature.

Copy link
Member

@lemeurherve lemeurherve Nov 4, 2022

Choose a reason for hiding this comment

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

Make perfect sense to have a third default state in that case indeed, I missed that requirement in your previous comment.

@timja timja added web-ui The PR includes WebUI changes which may need special expertise rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted labels Oct 23, 2022
@NotMyFault NotMyFault self-requested a review October 24, 2022 21:36
Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

Looking good (tested locally with some flags), do we need any documentation / guidance written for this?

@Wadeck
Copy link
Contributor Author

Wadeck commented Nov 4, 2022

do we need any documentation / guidance written for this?

TBH I do not know.
It's expected to be used by only a few people at this point, the API is not really complicated, so not sure it's worth the effort.

@NotMyFault
Copy link
Member

It's expected to be used by only a few people at this point, the API is not really complicated, so not sure it's worth the effort.

We could add a small paragraph to the design library outlining how to use this feature, could be useful I guess.

Copy link
Member

@uhafner uhafner left a comment

Choose a reason for hiding this comment

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

Wouldn't it make more sense to call it feature flag? The idea behind this concept is not only relevant to UI changes.

@Wadeck
Copy link
Contributor Author

Wadeck commented Nov 9, 2022

The idea behind this concept is not only relevant to UI changes.

This PR is only about user related flags. I was not able to find a non-UI feature that could be managed by users. For example, notification preference is not a flag, but more a settings. If you have examples of non-UI stuff, please tell me :))

In my mind, the "general" feature flag is more about enabling a particular feature to spread smoothly within an audience.

If you want to use equivalent that are for non-user feature, you already can use the SystemProperties (that we heavily use for security corrections with escape hatches)

Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

Good for now I think, we can adjust it as needed anyway.

@timja timja requested a review from a team January 2, 2023 18:07
@basil basil removed their request for review February 3, 2023 15:38
@timja
Copy link
Member

timja commented Mar 1, 2023

/label ready-for-merge


This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

@comment-ops-bot comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Mar 1, 2023
@NotMyFault NotMyFault merged commit af09d67 into jenkinsci:master Mar 12, 2023
<select class="jenkins-select__input" name="[${it.flagKey}]">
<f:option selected="${flagValue == null}" value="">
<j:if test="${it.getDefaultValue() == true}">${%Default_True}</j:if>
<j:if test="${it.getDefaultValue() == false}">${%Default_False}</j:if>
Copy link
Member

Choose a reason for hiding this comment

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

https://issues.jenkins.io/browse/JENKINS-71211

doesn't fit on mac 13" screen

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted web-ui The PR includes WebUI changes which may need special expertise
Projects
None yet
5 participants