I have done some work today to add a LDAP server and test cases to the Lucee 6.0 build GitHub Action
https://luceeserver.atlassian.net/browse/LDEV-4117
but it’s failing with a simple bind error, more deets in the PR
update: as per the PR and ticket, this is merged and disabled, you can just fork 6 and play with the config if you want to help
Anyone? Reckon the last time I used CFLDAP was in the 90s with Allaire and CF was still written in C++
How does this ticket detail meeting any sense of “definition of ready”?
Why are you even working on it?
This doesn’t answer your question, I get that, but WTH are you even doing starting to work on a ticket as ill-defined as this?
If you look at the ticket, the labels explain it’s about the build and tests.
There is a linked ticket about removing some old jars which aren’t used anymore and aren’t loaded by OSGI on demand. this means they are always loaded into memory, so removing them reduces the runtime memory usage of Lucee and reduces the disk footprint as well.
Besides, you’re the last person I’d expect to be complaining about adding test coverage to Lucee?
Both of these are especially important for making AWS Lambda and https://fuseless.org/ perform better. I have been discussing improving this with @pfreitag for a long time
I solved the problem by switching docker images.
anyway, IMHO code speaks louder than words. here’s the changes
Glad you got it.
As a newer member I have been trying to follow along, see how work is selected, prioritized, and completed.
Generally - how can I help?
This context is helpful.
I missed the linked ticket and even following it, doesn’t seem to include the “so memory usage and disk footprint is reduced which is key for goals (include link to goals).”
I wonder if a copy/paste of the important parts of these (out of band?) conversations may be helpful context for a future you or me?
A lot of the time, it’s a combination of responding to issues users encounter and raise (or myself in day to day developing with Lucee) I’m still more of a cfml dev than java dev, so I tinker with java stuff on Lucee, @micstriit does all the heavy lifting and @cfmitrah does a lot too.
Priorities are obviously regressions, things which are confusing / frustrating / cause dataloss / etc (i.e. docs, exception messages), or just good addressing old tech debt like this round of work I was doing.
The immediate way you and anyone else can help is jumping in and testing any tickets in QA status, creating tests for tickets without tests, updating / improving documentation.
We use labels quite heavily in Jira, so the labels on the ticket memShrink and shrink (disk) indicate this, smaller and faster are always general goals!
Alas, I’m not going to be sharing all my private conversions with people, when there is an outcome / direction / breaking which is important for others to know, I usually share them either by tickets and/or posts here.
So @Phillyun, welcome to the Lucee community and feel free to ask any further questions!
To be clear, I’m not suggesting whatever the work is isn’t important. It’s just impossible to tell from how the ticket is written. Sure there’s a linked ticket, but there’s not even a note saying “this is to do the x for the other linked ticket that does y”. How does the person reviewing your work here know what the deliverables are? How can they test that you’ve delivered them?
So any code review comments on the changes?
Purpose of the ticket was test coverage for CFLDAP
Someone could add ssl coverage if they want to chip in…
GitHub is terrible on mobile (thought I added and it didn’t stick) so I’ma be lazy and say here: line 53 of LDAP is skip=true instead of isDisabled() and I’m not sure why.
good catch!
OK. All the points below are trivial, and do not represent work I’d push back on, but just seed as “food for thought”.
Looking at the first commit, just stylistically I might flip the condition here and early-exit, so the dev has less code to look at before they get to the end of the conditional block:
if ( structCount( LDAP ) neq 5 ){
throw "not configured";
}
// rest of logic is now not conditional.
Do you wanna var that ldap variable here? I’m guessing yer relying on the application running in modern mode, but I don’t think one should ever do that? Although might be in your guidelines that it must do. Seems a touch risky and less clear than it could be, anyhow.
You are repeating that 5 magic number a coupla times. Initially I wondered whether giving it a clear label might help, but at least in the first instance it’s clear from the next statement what 5 is. Less so in the second instance where there’s no indication of why 5 is correct and 6 or 4 wouldn’t be. And of course you currently have two copies of that same number to keep track of (here and here)
The tests seem fine. I don’t know from the ticket detail [cough] whether yer covering everything you need to there.
In this commit, you have committed a bunch of commented-out code. Less than ideal.If yer line of thinking “someone might need this later”, well: someone also might not. And they can put the debugging back in exactly where they need it, rather than where you used to need it if needs must.
This is the only bit that if it was one of our code reviews at our shop, I’d be going “get rid”.
Everything else seems as legit as I can determine it to be based on the info provided.
–
Adam
cool thanks for the review!
those functions have localmode=true so there’s no need for var or local.
there actually was even no need for the structCount check, as the rule (how many config properties are required) checked when fetching the properties here
if the check isn’t met, an empty struct is returned and that test isn’t even reached if that condition isn’t met
Argh! Didnae notice that. All good then!