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

Is ident package use #1042

Merged
merged 28 commits into from
Mar 30, 2022
Merged

Is ident package use #1042

merged 28 commits into from
Mar 30, 2022

Conversation

tznind
Copy link
Contributor

@tznind tznind commented Feb 10, 2022

Proposed Changes

Replaces IsIdentifiable code in SmiServices with the same (refactored) code in the nuget repository.

Significant changes will need to be made to CI and default.yaml

  • We need to manage deployment of ii command line for reviewing IsIdentifiable results and doing on demand scans
  • We need to update our docs to say to use the IsIdentifiable command line instead of SmiRunner cli
  • default.yaml previously had 2 big options (Reviewer and IsIdentifiable). Now it will have 2
    • IsIdentifiableServiceOptions (contains rabbitMQ IConsumer options etc)
    • IsIdentifiableBaseOptions (sourced from the nuget package - rules dir, OCR options etc)
    • Reviewer options will disapear from default.yaml since we no longer run reviewer from from SmiRunner. Instead we will put the yaml thats currently in there into a seperate file for use with the ii command line tool.
  • Whitelist is now Allowlist (affects yaml settings files)
  • Redlist is now Reportlist (affects yaml settings files)
  • No longer supports specifying command line options when running service. All options are now specified only in the -y someOtherNonDefault.yaml. The ii tool has its own yaml and CLI args that still work as normal (i.e. like before).

Types of changes

What types of changes does your code introduce? Tick all that apply.

  • Bugfix (non-breaking change which fixes an issue)
  • New Feature (non-breaking change which adds functionality)
  • Breaking Change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation-Only Update (if none of the other choices apply)
    • In this case, ensure that the message of the head commit from the source branch is prefixed with [skip ci]

Checklist

By opening this PR, I confirm that I have:

  • Reviewed the contributing guidelines for this repository
  • Ensured that the PR branch is in sync with the target branch (i.e. it is automatically merge-able)
  • Updated any relevant API documentation
  • Created or updated any tests if relevant
  • Created a news file
    • NOTE: This must include any changes to any of the following files: default.yaml, any of the RabbitMQ server configurations, GlobalOptions.cs
  • Listed myself in the CONTRIBUTORS file 🚀
  • Requested a review by one of the repository maintainers

Issues

If relevant, tag any issues that are expected to be resolved with this PR. E.g.:

  • Closes #<issue-number>
  • ...

…l code

Building but not passing tests, still need to rework default.yaml and cli options for the service
@lgtm-com
Copy link

lgtm-com bot commented Feb 10, 2022

This pull request introduces 9 alerts and fixes 4 when merging a989902 into 38b900d - view on LGTM.com

new alerts:

  • 6 for Constant condition
  • 3 for Useless assignment to local variable

fixed alerts:

  • 2 for Constant condition
  • 1 for Call to GC.Collect()
  • 1 for Useless assignment to local variable

@lgtm-com
Copy link

lgtm-com bot commented Feb 10, 2022

This pull request introduces 9 alerts and fixes 4 when merging 174afa1 into 38b900d - view on LGTM.com

new alerts:

  • 6 for Constant condition
  • 3 for Useless assignment to local variable

fixed alerts:

  • 2 for Constant condition
  • 1 for Call to GC.Collect()
  • 1 for Useless assignment to local variable

@lgtm-com
Copy link

lgtm-com bot commented Feb 10, 2022

This pull request introduces 9 alerts and fixes 4 when merging 58db368 into 38b900d - view on LGTM.com

new alerts:

  • 6 for Constant condition
  • 3 for Useless assignment to local variable

fixed alerts:

  • 2 for Constant condition
  • 1 for Call to GC.Collect()
  • 1 for Useless assignment to local variable

@tznind tznind marked this pull request as ready for review February 10, 2022 12:39
@tznind tznind requested a review from rkm February 10, 2022 12:39
Fixed link to reviewer docs
@lgtm-com
Copy link

lgtm-com bot commented Feb 10, 2022

This pull request introduces 9 alerts and fixes 4 when merging 6b21c1a into 38b900d - view on LGTM.com

new alerts:

  • 6 for Constant condition
  • 3 for Useless assignment to local variable

fixed alerts:

  • 2 for Constant condition
  • 1 for Call to GC.Collect()
  • 1 for Useless assignment to local variable

@rkm
Copy link
Member

rkm commented Feb 10, 2022

Great stuff Thomas - I'll try and take a look at the CI failures tomorrow. Also enjoying the +110 -6,195 line diff!

@lgtm-com
Copy link

lgtm-com bot commented Feb 11, 2022

This pull request introduces 9 alerts and fixes 4 when merging 075b11c into b0953a1 - view on LGTM.com

new alerts:

  • 6 for Constant condition
  • 3 for Useless assignment to local variable

fixed alerts:

  • 2 for Constant condition
  • 1 for Call to GC.Collect()
  • 1 for Useless assignment to local variable

@lgtm-com
Copy link

lgtm-com bot commented Feb 11, 2022

This pull request introduces 9 alerts and fixes 4 when merging c2c1db3 into b0953a1 - view on LGTM.com

new alerts:

  • 6 for Constant condition
  • 3 for Useless assignment to local variable

fixed alerts:

  • 2 for Constant condition
  • 1 for Call to GC.Collect()
  • 1 for Useless assignment to local variable

@tznind tznind marked this pull request as draft February 16, 2022 14:38
@tznind
Copy link
Contributor Author

tznind commented Feb 16, 2022

Marking this draft again until I update it to a new refactored package of IsIdentifiable (2.1 standard - see SMI/IsIdentifiable#22)

@tznind tznind marked this pull request as ready for review March 1, 2022 08:54
@lgtm-com
Copy link

lgtm-com bot commented Mar 1, 2022

This pull request introduces 18 alerts and fixes 53 when merging 6bb3ba2 into 30719be - view on LGTM.com

new alerts:

  • 9 for Use of default ToString()
  • 6 for Constant condition
  • 3 for Useless assignment to local variable

fixed alerts:

  • 51 for Use of default ToString()
  • 1 for Constant condition
  • 1 for Dereferenced variable may be null

@lgtm-com
Copy link

lgtm-com bot commented Mar 3, 2022

This pull request introduces 18 alerts and fixes 53 when merging b33825f into c466609 - view on LGTM.com

new alerts:

  • 9 for Use of default ToString()
  • 6 for Constant condition
  • 3 for Useless assignment to local variable

fixed alerts:

  • 51 for Use of default ToString()
  • 1 for Constant condition
  • 1 for Dereferenced variable may be null

@tznind
Copy link
Contributor Author

tznind commented Mar 3, 2022

Possibly differing versions of tesseract? or is the IsIdentifiable nuget package borked with replication?

Error: /home/runner/.dotnet/sdk/6.0.102/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.ConflictResolution.targets(112,5): error NETSDK1152: Found multiple publish output files with the same relative path: /home/runner/.nuget/packages/tesseract/4.1.0-beta1/x86/tesseract41.dll, /home/runner/.nuget/packages/tesseract/4.1.1/x86/tesseract41.dll, /home/runner/.nuget/packages/tesseract/4.1.0-beta1/x64/tesseract41.dll, /home/runner/.nuget/packages/tesseract/4.1.1/x64/tesseract41.dll. [/home/runner/work/SmiServices/SmiServices/src/applications/Applications.SmiRunner/Applications.SmiRunner.csproj]

Looks like a change for net6.0 where previously it was a silent overwrite:

https://docs.microsoft.com/en-us/dotnet/core/compatibility/sdk/6.0/duplicate-files-in-output

@lgtm-com
Copy link

lgtm-com bot commented Mar 4, 2022

This pull request introduces 18 alerts and fixes 53 when merging d1d5241 into 2ee8301 - view on LGTM.com

new alerts:

  • 9 for Use of default ToString()
  • 6 for Constant condition
  • 3 for Useless assignment to local variable

fixed alerts:

  • 51 for Use of default ToString()
  • 1 for Constant condition
  • 1 for Dereferenced variable may be null

@lgtm-com
Copy link

lgtm-com bot commented Mar 4, 2022

This pull request introduces 18 alerts and fixes 53 when merging 2d0a3c6 into 2ee8301 - view on LGTM.com

new alerts:

  • 9 for Use of default ToString()
  • 6 for Constant condition
  • 3 for Useless assignment to local variable

fixed alerts:

  • 51 for Use of default ToString()
  • 1 for Constant condition
  • 1 for Dereferenced variable may be null

This reverts commit 38df36a.
@lgtm-com
Copy link

lgtm-com bot commented Mar 4, 2022

This pull request introduces 18 alerts and fixes 53 when merging 57bcfca into 2ee8301 - view on LGTM.com

new alerts:

  • 9 for Use of default ToString()
  • 6 for Constant condition
  • 3 for Useless assignment to local variable

fixed alerts:

  • 51 for Use of default ToString()
  • 1 for Constant condition
  • 1 for Dereferenced variable may be null

@lgtm-com
Copy link

lgtm-com bot commented Mar 7, 2022

This pull request introduces 18 alerts and fixes 53 when merging a845c2a into 6b66f9b - view on LGTM.com

new alerts:

  • 9 for Use of default ToString()
  • 6 for Constant condition
  • 3 for Useless assignment to local variable

fixed alerts:

  • 51 for Use of default ToString()
  • 1 for Constant condition
  • 1 for Dereferenced variable may be null

@lgtm-com
Copy link

lgtm-com bot commented Mar 7, 2022

This pull request introduces 18 alerts and fixes 53 when merging 9b04e12 into 73e978c - view on LGTM.com

new alerts:

  • 9 for Use of default ToString()
  • 6 for Constant condition
  • 3 for Useless assignment to local variable

fixed alerts:

  • 51 for Use of default ToString()
  • 1 for Constant condition
  • 1 for Dereferenced variable may be null

@lgtm-com
Copy link

lgtm-com bot commented Mar 9, 2022

This pull request introduces 18 alerts and fixes 53 when merging 4101890 into 910da10 - view on LGTM.com

new alerts:

  • 9 for Use of default ToString()
  • 6 for Constant condition
  • 3 for Useless assignment to local variable

fixed alerts:

  • 51 for Use of default ToString()
  • 1 for Constant condition
  • 1 for Dereferenced variable may be null

@lgtm-com
Copy link

lgtm-com bot commented Mar 9, 2022

This pull request introduces 18 alerts and fixes 53 when merging 8c1b58e into 910da10 - view on LGTM.com

new alerts:

  • 9 for Use of default ToString()
  • 6 for Constant condition
  • 3 for Useless assignment to local variable

fixed alerts:

  • 51 for Use of default ToString()
  • 1 for Constant condition
  • 1 for Dereferenced variable may be null

@lgtm-com
Copy link

lgtm-com bot commented Mar 9, 2022

This pull request introduces 18 alerts and fixes 53 when merging a951bfc into 910da10 - view on LGTM.com

new alerts:

  • 9 for Use of default ToString()
  • 6 for Constant condition
  • 3 for Useless assignment to local variable

fixed alerts:

  • 51 for Use of default ToString()
  • 1 for Constant condition
  • 1 for Dereferenced variable may be null

@lgtm-com
Copy link

lgtm-com bot commented Mar 9, 2022

This pull request introduces 18 alerts and fixes 53 when merging b21db19 into 910da10 - view on LGTM.com

new alerts:

  • 9 for Use of default ToString()
  • 6 for Constant condition
  • 3 for Useless assignment to local variable

fixed alerts:

  • 51 for Use of default ToString()
  • 1 for Constant condition
  • 1 for Dereferenced variable may be null

@lgtm-com
Copy link

lgtm-com bot commented Mar 9, 2022

This pull request introduces 18 alerts and fixes 53 when merging 3916455 into 910da10 - view on LGTM.com

new alerts:

  • 9 for Use of default ToString()
  • 6 for Constant condition
  • 3 for Useless assignment to local variable

fixed alerts:

  • 51 for Use of default ToString()
  • 1 for Constant condition
  • 1 for Dereferenced variable may be null

@lgtm-com
Copy link

lgtm-com bot commented Mar 15, 2022

This pull request introduces 18 alerts and fixes 53 when merging 1b9f44f into 999647c - view on LGTM.com

new alerts:

  • 9 for Use of default ToString()
  • 6 for Constant condition
  • 3 for Useless assignment to local variable

fixed alerts:

  • 51 for Use of default ToString()
  • 1 for Constant condition
  • 1 for Dereferenced variable may be null

@lgtm-com
Copy link

lgtm-com bot commented Mar 16, 2022

This pull request introduces 18 alerts and fixes 53 when merging 57be31a into 999647c - view on LGTM.com

new alerts:

  • 9 for Use of default ToString()
  • 6 for Constant condition
  • 3 for Useless assignment to local variable

fixed alerts:

  • 51 for Use of default ToString()
  • 1 for Constant condition
  • 1 for Dereferenced variable may be null

@tznind tznind mentioned this pull request Mar 30, 2022
@lgtm-com
Copy link

lgtm-com bot commented Mar 30, 2022

This pull request introduces 18 alerts and fixes 53 when merging b7d4737 into bd05e5b - view on LGTM.com

new alerts:

  • 9 for Use of default ToString()
  • 6 for Constant condition
  • 3 for Useless assignment to local variable

fixed alerts:

  • 51 for Use of default ToString()
  • 1 for Constant condition
  • 1 for Dereferenced variable may be null

@jas88 jas88 merged commit 5189275 into master Mar 30, 2022
@jas88 jas88 deleted the is-ident-package-use branch March 30, 2022 15:03
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.

None yet

3 participants