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

Issues with Analysis SI #7

Open
paigeduffey opened this issue Oct 29, 2019 · 10 comments
Open

Issues with Analysis SI #7

paigeduffey opened this issue Oct 29, 2019 · 10 comments

Comments

@paigeduffey
Copy link

There seem to be some issues with the Analysis SI (sys_app.do?sys_id=0f7b73c4dba433007d159235ca96197c)

  • Takes an exceedingly long time to run when saving large code blocks (to be expected, but a bit of a bother when you're trying to make some tweaks to a large script include... even when trying to update the Analysis SI it took some time to update... at least 60+ seconds)
  • Regularly pop-ups an error about "do not hard code sys_ids" even when no hard coded sys_id's exist in the code (again it popped this code up even on the Analysis SI)
@jacebenson
Copy link
Owner

There must be a better way to handle this, so the code isn't slow...
A workaround is to not use this code analysis. But that I don't think is acceptable.
This is a great call out. The sys_id code is using the regex here;
https://github.com/jacebenson/servicenow-code/blob/master/update/sys_script_include_c5c833f54f55f700c660b1d18110c78e.xml#L141

/('|"|`)[a-f0-9]{32}('|"|\`)/gm

I'm trying to think of ways to do this differently. Perhaps live updates of issues is not feasible and only a report against logs is a better way. What do you think Paige? No one else is piping up, so your input is appreciated.

@paigeduffey
Copy link
Author

Yea. I certainly think the real time alerts are wonderful in concept, but they were causing some major lag!

An after the fact code analysis would be really cool. There's something out on the store for free right now (By Evergreen), but I think you're doing some checks that they do not do (like hard coding sysids :) ) that would be really nice.

We primarily grabbed this for the code search (which is fab, btw), so this analysis was an added bonus. Always looking for things to make our Code Reviews go faster though (especially as we bring on more citizen developers).

@jacebenson
Copy link
Owner

jacebenson commented Oct 30, 2019

I think that’s great you are using it. I’ve been meaning to expand the tests to include all the “Ace” report findings but I have yet to get a copy of one to get their list.

So here’s my thoughts on the lag.

  • drop the real-time lookups. Instead do them ;
    • ad-hoc via a ui action or
    • onload client script (I don’t like this idea) or
    • dedicated sp page to show records with issues

@paigeduffey
Copy link
Author

I do like the UI Action idea! Run it when you want to run it.

@jacebenson
Copy link
Owner

Sounds good. I'll make the change. Keep the property to control visibility.

@jacebenson
Copy link
Owner

Seems that for UI Actions and client scripts I can't pick business rules, script includes or client scripts... So that leaves a dedicated sp page or instructions to add a ui aciton to global scope.

@paigeduffey
Copy link
Author

Hmm...what about a Script Action? (UI Action triggers event, even triggers Script Action)

@jacebenson
Copy link
Owner

That's the problem. On UI Action I can't tell it to exist on the business rule or script include tables.

image

@jacebenson
Copy link
Owner

I wonder in looking at th script include br i see problems.. maybe correcting them will make it more performant

@jacebenson
Copy link
Owner

In the mean time I think I corrected a error on the Analysis script include so it should inspect the one script include, not all of them and that may be faster.

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

No branches or pull requests

2 participants