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

add configuration-as-code plugin to Bill of Material #78

Merged
merged 7 commits into from
Sep 18, 2019
Merged

add configuration-as-code plugin to Bill of Material #78

merged 7 commits into from
Sep 18, 2019

Conversation

jetersen
Copy link
Member

cc @jglick @oleg-nenashev hopefully I did this right 😅

jglick
jglick previously requested changes Aug 27, 2019
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Triggers a test failure in git. Probably something for @MarkEWaite to fix (incompatibility with newer versions of configuration-as-code?) but TBD. If the failure is triaged and fixed in a separate PR, then we can go ahead with this after excluding that test suite until the other PR is merged and released.

bom/pom.xml Outdated Show resolved Hide resolved
bom/pom.xml Outdated Show resolved Hide resolved

@Test
public void smokes() {
assertThat(Jenkins.get().getSystemMessage(), is("Hello World"));
Copy link
Member

Choose a reason for hiding this comment

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

A bit more interesting to configure some object defined in this plugin, but good enough for now I guess.

Copy link
Member Author

@jetersen jetersen Aug 27, 2019

Choose a reason for hiding this comment

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

There is nothing really interesting to configure in JCasC 😆
Though we could use it to smoke test all other plugins 😆

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, currently this plugin does not define any global settings, but it could. Whether that would make test coverage any more meaningful for BOM updates is another question.

Copy link
Member Author

Choose a reason for hiding this comment

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

91d7c3e hmm
@MarkEWaite 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

Just wanna state BOM and PCT is pretty awesome! Kudos 😍

Copy link
Member Author

Choose a reason for hiding this comment

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

Should I remove the test?

Copy link
Member

Choose a reason for hiding this comment

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

It is fine.

@MarkEWaite
Copy link
Contributor

That test failure is fixed in git client plugin 2.8.2. The JCasC plugin updated its test definition and the git client plugin was updated to use the most recent JCasC plugin tests.

Co-Authored-By: Jesse Glick <[email protected]>
@jglick
Copy link
Member

jglick commented Aug 28, 2019

Well that fixed the git failure; now we have other failures related to configuration-as-code:

  • ssh-credentials was apparently testing against a version predating a security fix.
  • ssh-slaves fails for some reason not immediately obvious to me.

@jetersen
Copy link
Member Author

Same issue. They both need updating of JCasC just as Git did.

@jetersen
Copy link
Member Author

@MRamonLeon are you up for fixing them? 😅

@MRamonLeon
Copy link

Sorry @Casz, I don't have enough bandwidth right now, maybe next week.
For ssh-slaves, looks like JCasC is not executing after the restart, there isn't any log of JCasC.

For ssh-credentials, maybe a security issue has been solved and the log doesn't contain the plain password anymore and the test needs to be fixed:
The log should have 'passphrase = passphrase-of-userid2'

0.690 [id=693] FINE i.j.p.c.i.c.DataBoundConfigurator#tryConstructor: Setting class com.cloudbees.jenkins.plugins.sshcredentials.impl.BasicSSHUserPrivateKey. passphrase = ****

Please take into account that the RoundTripAbstractTest test I did is just a proxy, a utility to develop JCasC complete tests quickly, so it's going to be in the middle of every failure, but don't shoot the messenger 😅

@jetersen
Copy link
Member Author

@MRamonLeon I'll see if I have bandwidth for it 😅

@jetersen
Copy link
Member Author

jetersen commented Sep 8, 2019

Looking into ssh-credentials 😓

@timja
Copy link
Member

timja commented Sep 8, 2019

Looking into ssh-credentials 😓

hah, I just created a branch on that repo, it'll require this to be merged:
jenkinsci/configuration-as-code-plugin#1049

@jetersen
Copy link
Member Author

jetersen commented Sep 8, 2019

Okay I'll look into the other one @timja so looking into ssh-slaves 😓

@jetersen
Copy link
Member Author

jetersen commented Sep 8, 2019

I see someone else did my work: jenkinsci/ssh-agents-plugin#147 thanks @fcojfernandez

@jetersen
Copy link
Member Author

@jglick would you prefer dependabot updating the deps or are you okay with me updating the deps in this PR?

@jglick
Copy link
Member

jglick commented Sep 10, 2019

If you have some dependencies related to this change that you see could be updated, by all means update them.

@jglick jglick added the enhancement New feature or request label Sep 11, 2019
@jetersen
Copy link
Member Author

jetersen commented Sep 13, 2019

@MarkEWaite any idea why these fail consistently:

pct-git-client / test_getAllLogEntries – org.jenkinsci.plugins.gitclient.JGitAPIImplTest
pct-git-client / test_getAllLogEntries – org.jenkinsci.plugins.gitclient.JGitApacheAPIImplTest

https://ci.jenkins.io/blue/organizations/jenkins/Tools%2Fbom/detail/PR-78/11/tests ?

@MarkEWaite
Copy link
Contributor

MarkEWaite commented Sep 13, 2019

@Casz the getAllLogEntries test compares the output of git log --all origin/master between command line git and JGit. The JGit 4.5 and JGit 5.4 output differs from the command line git output when two commits have a timestamp of the same second.

I've disabled the assertion in that test on the development branches (master and stable-2.8) of the git client plugin because the test is checking a deprecated method and is not very useful. However, the released git client plugin 2.8.5 includes that test and now fails due to the recent commits on the master branch.

If you need a git client plugin release that includes that change, you can either use git client plugin 3.0.0-beta11 or an incremental build from the master branch.

bom/pom.xml Outdated Show resolved Hide resolved
@jetersen
Copy link
Member Author

🙏 for green

@jetersen
Copy link
Member Author

jetersen commented Sep 16, 2019

@jglick not sure the CI is so red 😕❓

@jglick jglick self-requested a review September 16, 2019 17:39
@jglick
Copy link
Member

jglick commented Sep 16, 2019

the CI is so red

Sigh, yes, seems like Remoting problems on ci.jenkins.io again.

@timja
Copy link
Member

timja commented Sep 17, 2019

the CI is so red

Sigh, yes, seems like Remoting problems on ci.jenkins.io again.

I can't even load this CI job =/, it's been loading for +20 mins

@jetersen jetersen closed this Sep 18, 2019
@jetersen jetersen reopened this Sep 18, 2019
@timja
Copy link
Member

timja commented Sep 18, 2019

🙏

@timja
Copy link
Member

timja commented Sep 18, 2019

Whoop green, cc @jglick

@jetersen
Copy link
Member Author

This would be great to get merged!

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for all downstream changes to make it happen!

@jglick jglick merged commit b10cc76 into jenkinsci:master Sep 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants