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-62739 Remove Bootstrap CSS #60

Merged
merged 16 commits into from
Jul 21, 2020

Conversation

aheritier
Copy link
Member

@aheritier aheritier commented Jun 21, 2020

This PR is upgrading jenkins version requirement from 2.138.4 to 2.204.6 + remove the BootStrap CSS to use the native ones proposed by Jenkins

It fixes #61 #62 #63 #64 #65

@fqueiruga I wasn't able to use CSS variables like you proposed in https://issues.jenkins-ci.org/browse/JENKINS-62739 because (at least with 2.204.6) it overrides by empty the first value. It gives:

Screenshot 2020-07-20 16 04 16

Changes:

  • JENKINS-62739 Remove Bootstrap CSS
  • Bump jenkins version requirement from 2.138.4 to 2.204.6
  • Bump hamcrest-library from 1.3 to 2.2
  • Bump plugin from 3.50 to 4.4
  • Bump gson from 2.8.1 to 2.8.6
  • Bump hamcrest-core from 1.3 to 2.2
  • Bump support-core from 2.63 to 2.69

Result

Configuration reminder

Screenshot 2020-07-20 16 01 44

Configuration to do

Screenshot 2020-07-20 16 02 17

Configuration done

Screenshot 2020-07-20 16 07 20

With 2.245 and Dark Theme

It looks ok with the Dark Theme

FYI @timja

Screenshot 2020-07-20 16 26 53

Screenshot 2020-07-20 16 27 19

Screenshot 2020-07-20 16 28 28

@aheritier aheritier self-assigned this Jun 21, 2020
dependabot-preview bot and others added 11 commits July 20, 2020 12:52
Bumps [gson](https://github.com/google/gson) from 2.8.1 to 2.8.6.
- [Release notes](https://github.com/google/gson/releases)
- [Changelog](https://github.com/google/gson/blob/master/CHANGELOG.md)
- [Commits](google/gson@gson-parent-2.8.1...gson-parent-2.8.6)

Signed-off-by: dependabot-preview[bot] <support@dependabot.com>
+ Bump plugin from 3.50 to 4.4
+ Bump hamcrest-core from 1.3 to 2.2
+ Bump hamcrest-library from 1.3 to 2.2
+ Wrap all the main-panel part in a specific div
+ Create a local CSS for the missing `alert-success` style (ref JENKINS-62747)
@aheritier aheritier marked this pull request as ready for review July 20, 2020 14:30
@aheritier aheritier requested a review from kwhetstone as a code owner July 20, 2020 14:30
@aheritier aheritier closed this Jul 20, 2020
@aheritier aheritier reopened this Jul 20, 2020
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.

Thanks for checking dark theme, FTR alerts are being re-themed, and will look different in 2.246 being released tomorrow (for dark theme only)

hopefully some of the contrast issues will be fixed in that

@aheritier
Copy link
Member Author

I don't understand why the build is failing on J11. It seems we are passing in the command line -Djenkins.version=2.164.1 which is the root cause of the error because the support-core plugin and this plugin are now based on 2.204.6.

Not sure where thus parameter is coming from. Maybe @jglick or @oleg-nenashev know ?

Co-authored-by: Tim Jacomb <t.jacomb@kainos.com>
@timja
Copy link
Member

timja commented Jul 20, 2020

https://github.com/jenkinsci/cloudbees-jenkins-advisor-plugin/blob/master/Jenkinsfile

Change it to be just:

buildPlugin()

FTR:
jenkins-infra/pipeline-library#145

@aheritier
Copy link
Member Author

@timja 👍 (We really have to keep an eye everywhere)

@timja
Copy link
Member

timja commented Jul 20, 2020

https://ci.jenkins.io/blue/organizations/jenkins/Plugins%2Fcloudbees-jenkins-advisor-plugin/detail/PR-60/5/tests

likely needs a test adjustment to allow files to be deleted properly on windows, there were some changes in jenkins test harness to surface these errors better I think

@aheritier
Copy link
Member Author

OMG ... Windows.. Why do we still support this crappy stuff
I will look at it tomorrow, I already spent more thime than expected on it today.
Thanks a lot for your help @timja

@aheritier
Copy link
Member Author

it looks to be related to jenkinsci/jenkins-test-harness#185
I have no idea why all these files are still locked.
Not sure what to do.
@jtnord @jglick is there a good practice to implement or fix to do to have a build more windows friendly ?

@jglick
Copy link
Member

jglick commented Jul 20, 2020

Also jenkinsci/jenkins-test-harness#198 perhaps. What is the problem you are seeing? Maybe some bug in your tests?

@jglick
Copy link
Member

jglick commented Jul 20, 2020

Not sure what

@Test
@WithPluginManager(DisabledPluginManager.class)
public void execute_pluginDisabled() {
BundleUpload subject = j.getInstance().getExtensionList(BundleUpload.class).get(0);
stubFor(any(anyUrl()));
subject.run();
verify(0, anyRequestedFor(anyUrl()));
}
is doing but might need to be suppressed on Windows unless you can track down a specific problem.

Anyway why do you need mocks in

? You could just use @LocalData with a *.jpi.disabled file.

@aheritier
Copy link
Member Author

@jglick I didn't write this code. I will try to understand better its purpose

AFAIR execute_pluginDisabled was written to verify that nothing is sent to Advisor server when the plugin is disabled (I remember some bugs at the origin of the plugin which was sending a bundle when the plugin wasn't yet configured. But I am not sure about the interest of doing such test when the plugin is disabled)

For

no idea at all

Use @LocalData to disable the plugin instead of a specific PluginManager

(With the hope it makes the test more Windows friendly on our CI env)
@aheritier
Copy link
Member Author

@jglick is the best (like often) !
I updated/simplified the test to use a "@LocalData" to disable the plugin and windows prefers it.

@jtnord
Copy link
Member

jtnord commented Jul 21, 2020

the standard test plugin manager does not support disabling plugins at run time. this is because it is reused across tests for speed and does not close the jars that the plugin depends on.

@aheritier
Copy link
Member Author

@jtnord great to know. Thanks

@fqueiruga
Copy link
Contributor

Suggesting changes to allow theming as progressive enhancement with fallback values. I think it should work. (Not a blocker)

@aheritier
Copy link
Member Author

aheritier commented Jul 21, 2020

@fqueiruga I tried it by copying what you proposed in Jira but it gave me that:

Am I doing something wrong ?

I was test with mvn hpi:run thus with 2.204.6

Co-authored-by: Félix Queiruga <felix.queiruga@gmail.com>
@aheritier
Copy link
Member Author

Not sure what I did wrong previously @fqueiruga @timja but it's all good with your patch using the CSS variables

2.204.6

Screenshot 2020-07-21 12 14 53

2.245 + Dark Theme

Screenshot 2020-07-21 12 18 54

It looks good to me

@aheritier aheritier requested a review from fqueiruga July 21, 2020 10:22
@fqueiruga
Copy link
Contributor

To disect the changes a bit,

// Fallback value for IE11
border-color: #c3e6cb;  

// Use the the proper CSS variable and if it is not defined, fall back to using #c3e6cb
// As this declaration is invalid in IE11 it will fall back to the previous one
border-color: var(--alert-success-border-color, #c3e6cb);  

Copy link
Contributor

@fqueiruga fqueiruga left a comment

Choose a reason for hiding this comment

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

Haven't been able to test it locally. If you haven't I recommend testing it on IE 11 as well :)

@aheritier
Copy link
Member Author

ahhh I better understand @fqueiruga
In https://issues.jenkins-ci.org/browse/JENKINS-62739 you gave me

.alert-success {
  background-color: #d4edda;
  background-color: var(--alert-success-bg-color);
  border-color: #c3e6cb;
  border-color: var(--alert-success-border-color);
  color: #138347;
  color: var(--alert-success-color);  
}

There was no fallback in the var declaration thus my issue in the rendering and the missing color

If you haven't I recommend testing it on IE 11 as well :)

AH AH AH IE 11

I think I will do the release and I'll see if I get some bugs reports :-)

@aheritier aheritier merged commit a0144cc into jenkinsci:master Jul 21, 2020
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.

5 participants