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.

Sign up for GitHub

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

Jump to bottom

[JENKINS-57121, JENKINS-57567] - make list view JCasC compliant #3994

Merged
merged 4 commits into from Jun 7, 2019
Merged

[JENKINS-57121, JENKINS-57567] - make list view JCasC compliant #3994

merged 4 commits into from Jun 7, 2019

Conversation

jetersen
Copy link
Member

@jetersen jetersen commented Apr 19, 2019 •
edited by oleg-nenashev

Fairly simple, adding some data binding to list view for JCasC to use.

I will add a downstream PR.

Proposed changelog entries

  • ?

Submitter checklist

  • JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@daniel-beck daniel-beck changed the title make list view JCasC complianant make list view JCasC compliant Apr 21, 2019
@daniel-beck
Copy link
Member

Would like the submit method actually use the new setters and work with them.

Also would like explicit confirmation the non-@DataboundSetter setRecurse works in CasC. Otherwise this is still missing too.

daniel-beck
daniel-beck previously requested changes Apr 21, 2019
View reviewed changes
Copy link
Member

@daniel-beck daniel-beck left a comment

Choose a reason for hiding this comment

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

above

@jetersen jetersen mentioned this pull request Apr 22, 2019
Closed
5 tasks
@jetersen
Copy link
Member Author

We now have a downstream PR: jenkinsci/configuration-as-code-plugin#850

@jetersen
Copy link
Member Author

Seems like Stapler decided to leave the building.
https://ci.jenkins.io/blue/organizations/jenkins/Plugins%2Fconfiguration-as-code-plugin/detail/PR-850/2/pipeline

anyone care to venture a guess? it seems to work at least for the integrations tests 😓

@jetersen
Copy link
Member Author

jetersen commented Apr 22, 2019 •
edited

@daniel-beck would it not be better to remove the submit and update the jelly to use the same data binding?

@daniel-beck
Copy link
Member

would it not be better to remove the submit and update the jelly to use the same data binding?

If you can achieve this without making the UI really weird, sure. Would expect that to be quite a bit more difficult though, for relatively little upside.

@jetersen
Copy link
Member Author

ya, I started looking and quickly said NOPE 😆

oleg-nenashev
oleg-nenashev requested changes Apr 26, 2019
View reviewed changes
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.

Whatever you try to synchronize here, AFAICT it won't work reliably in such way. The approach itself looks good to me

core/src/main/java/hudson/model/ListView.java Outdated Show resolved Hide resolved
@jetersen
Copy link
Member Author

jetersen commented May 20, 2019 •
edited

@daniel-beck @oleg-nenashev @jvz can you take another look

578c45e fixes JENKINS-57567 I can make it a separate PR but the downstream test is related.

Waiting on incremental to update downstream

@jetersen
Copy link
Member Author

jetersen commented May 20, 2019 •
edited

Not sure how to use snapshot/incremental of Jenkins Core, seems Stapler is a no show.

@jetersen
Copy link
Member Author

jetersen commented May 20, 2019 •
edited

Would like the submit method actually use the new setters and work with them.

@daniel-beck I would prefer not to change too much. You should have permissions to add commits if you have an idea on you would change the submit.

@jetersen
Copy link
Member Author

jetersen commented May 20, 2019 •
edited

... Deployed Snapshot and Incremental both fail to restore Stapler on downstream PR

@jetersen
Copy link
Member Author

Downstream PR is passing @daniel-beck @oleg-nenashev and tests all the new data-bound setters are working 😅

@timja
Copy link
Member

timja commented May 27, 2019

@daniel-beck / @oleg-nenashev would you be able to take another look please?

oleg-nenashev
oleg-nenashev approved these changes May 27, 2019
View reviewed changes
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. jobFilters is never null, and it should work. But we need a JIRA ticket to land it in the code.

It also would not hurt to start including JCasC autotests directly into the Jenkins Test Harness suite in the core.

@oleg-nenashev oleg-nenashev added the needs-jira-issue A JIRA issue should be created for this PR. It is usually needed for the LTS backporting process label May 27, 2019
@timja
Copy link
Member

timja commented May 27, 2019 •
edited

@oleg-nenashev seems one already exists:
https://issues.jenkins-ci.org/browse/JENKINS-57121

@jetersen
Copy link
Member Author

Two already exist this PR takes care of both:
https://issues.jenkins-ci.org/browse/JENKINS-57121
https://issues.jenkins-ci.org/browse/JENKINS-57567

Though https://issues.jenkins-ci.org/browse/JENKINS-57567 is super minor with only one databound setter 😕

@jetersen
Copy link
Member Author

@oleg-nenashev can we remove the needs-jira-issue and change it for ready for merge? 😓

@jetersen
Copy link
Member Author

jetersen commented May 27, 2019 •
edited

@daniel-beck care to dismiss or re-review the PR?

@timja timja mentioned this pull request Jun 1, 2019
Closed
@timja
Copy link
Member

timja commented Jun 1, 2019

@oleg-nenashev can we remove needs-jira-issue please
@daniel-beck would you be able to re-review please?

@jetersen jetersen changed the title make list view JCasC compliant [JENKINS-57121] - make list view JCasC compliant Jun 1, 2019
@oleg-nenashev oleg-nenashev dismissed daniel-beck’s stale review June 4, 2019 15:48

Compat issue was fixed, re-requesting review

@oleg-nenashev
Copy link
Member

Compat issue was fixed, and the fix looks good to me.
For the future... what if we include JCasC tests directly into the core test suite? It looks like JCasC plugin is getting enough traction to consider doing so right in the core so that we can discover issues early

@timja
Copy link
Member

timja commented Jun 4, 2019

Sounds like a good idea 👍

@jetersen
Copy link
Member Author

jetersen commented Jun 4, 2019

I'd be happy to add JCasC testing in core since it will make testing core changes a lot easier.

@oleg-nenashev
Copy link
Member

I am fine with adding a JCasC dependency to https://github.com/jenkinsci/jenkins/blob/master/test-pom/pom.xml so that we start improving JCasC compatibility test coverage and adding tests for regressions. @jenkinsci/core opinions?

Also CC @MRamonLeon @varyvol @fcojfernandez @alecharp who might be interested

@oleg-nenashev
Copy link
Member

Maybe dev list is a proper venue tho

@jetersen
Copy link
Member Author

jetersen commented Jun 4, 2019 •
edited

https://groups.google.com/forum/#!topic/jenkinsci-dev/cgbYEB3c_Vc 😅

MRamonLeon
MRamonLeon approved these changes Jun 4, 2019
View reviewed changes
Copy link
Contributor

@MRamonLeon MRamonLeon left a comment

Choose a reason for hiding this comment

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

it would help a lot to make JCasC always compatible with the latest changes in core. And vice-versa

@oleg-nenashev oleg-nenashev removed the needs-jira-issue A JIRA issue should be created for this PR. It is usually needed for the LTS backporting process label Jun 4, 2019
@oleg-nenashev
Copy link
Member

Anyway, we could detach the test automation to a separate pull request. Will merge towards the next weekly if no negative feedback

@oleg-nenashev oleg-nenashev added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Jun 4, 2019
@fcojfernandez
Copy link
Contributor

@oleg-nenashev Adding the dependency to the core makes sense for me. In this case, this feature is so core related that (I hope) there should be really little negative feedback.

@batmat batmat closed this Jun 5, 2019
@batmat batmat reopened this Jun 5, 2019
@oleg-nenashev oleg-nenashev merged commit 51e2cdc into jenkinsci:master Jun 7, 2019
@oleg-nenashev oleg-nenashev changed the title [JENKINS-57121] - make list view JCasC compliant [JENKINS-57121, JENKINS-57567] - make list view JCasC compliant Jun 7, 2019
@oleg-nenashev
Copy link
Member

JENKINS-57567 is also fixed

@jetersen jetersen deleted the fix/listViewDataBinding branch June 21, 2019 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jvz jvz jvz approved these changes

@oleg-nenashev oleg-nenashev oleg-nenashev approved these changes

@varyvol varyvol varyvol approved these changes

@MRamonLeon MRamonLeon MRamonLeon approved these changes

@fcojfernandez fcojfernandez fcojfernandez approved these changes

@daniel-beck daniel-beck Awaiting requested review from daniel-beck

Assignees
No one assigned
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback
Projects
None yet
Milestone
No milestone
9 participants
@jetersen @daniel-beck @timja @oleg-nenashev @fcojfernandez @jvz @varyvol @MRamonLeon @batmat