Skip to content
This repository was archived by the owner on Aug 20, 2021. It is now read-only.

add regex to get componentID from control mapping - #51

Merged
anweiss merged 3 commits into
docker-archive:masterfrom
aitchkhan:add-componentID-regex
Jan 25, 2019
Merged

add regex to get componentID from control mapping#51
anweiss merged 3 commits into
docker-archive:masterfrom
aitchkhan:add-componentID-regex

Conversation

@aitchkhan

Copy link
Copy Markdown
Contributor

add componentID from excel in generated implementation

@aitchkhan
aitchkhan force-pushed the add-componentID-regex branch from 0a026b5 to 97003ae Compare January 17, 2019 11:35
Comment thread impl/impl_test.go
Comment thread impl/implementation.go
Comment thread impl/implementation.go
@aitchkhan
aitchkhan force-pushed the add-componentID-regex branch from aed938d to 88ba100 Compare January 17, 2019 14:54

@anweiss anweiss left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@aitchkhan @haroonKhan-10p let's try to avoid hardcoding slice indices and instead use range to retrieve the item in question. It's an odd way of iterating.

@anweiss anweiss added the enhancement New feature or request label Jan 17, 2019
@aitchkhan

Copy link
Copy Markdown
Contributor Author

which slice indices are you referring to?
the one generating the componentDefiniton or the one consuming the index of excel sheet for extracting identifier of UCP?

@asadullah-yousuf-10p

Copy link
Copy Markdown
Contributor

which slice indices are you referring to?
the one generating the componentDefiniton or the one consuming the index of excel sheet for extracting identifier of UCP?

@anweiss

@anweiss

anweiss commented Jan 18, 2019

Copy link
Copy Markdown
Contributor

@aitchkhan @haroonKhan-10p @asadullah-yousuf-10p I'm referring to all of them lol. Remember, the CSV that we're using is temporary, at least until the OSCAL "implementation" schema has been fully developed. And even though we have control over the layout of our CSV and can make some guarantees as to the indices of the elements in the slices, other consumers of oscalkit won't have this knowledge. Furthermore, it's simply not really an idiomatic way of iterating through slices; especially in this case where we have multi-dimensional slices and our CSV is still changing. Easy to overlook an index and end up with an index out of range error.

@anweiss

anweiss commented Jan 23, 2019

Copy link
Copy Markdown
Contributor

@haroonKhan-10p if you could fix the iteration constructs by removing the hardcoded indices, we can go ahead and get this merged. thanks

@aitchkhan

aitchkhan commented Jan 23, 2019 via email

Copy link
Copy Markdown
Contributor Author

@mohuk

mohuk commented Jan 24, 2019

Copy link
Copy Markdown
Contributor

I believe indices were placed to get things rolling. A future proof solution would not be relying on the csv or google/excel sheets, instead it should be a xml schema listing down the controls and other data in a structured format. Same schema can then be handed over to consumers so they can create their own implementation and import it via oscalkit.

So for Amberjack, can we stick to what we have and add the xml part to the backlog ?

@anweiss

anweiss commented Jan 24, 2019

Copy link
Copy Markdown
Contributor

@mohuk @aitchkhan we can leave the hardcoded indices for now. Please rebase and resolve conflicts and I'll go ahead and merge. I'll create a separate issue for us to address this in the future when the OSCAL "implementation" model matures.

@anweiss

anweiss commented Jan 24, 2019

Copy link
Copy Markdown
Contributor

@aitchkhan @haroonKhan-10p please resolve conflicts and rebase.

Signed-off-by: aitchkhan <aitchkhan@gmail.com>
Signed-off-by: aitchkhan <aitchkhan@gmail.com>
@aitchkhan
aitchkhan force-pushed the add-componentID-regex branch from 88ba100 to 63287fc Compare January 25, 2019 03:16
@haroonKhan-10p

Copy link
Copy Markdown
Contributor

updated.

Signed-off-by: aitchkhan <aitchkhan@gmail.com>

@anweiss anweiss left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@anweiss
anweiss dismissed minhaj10p’s stale review January 25, 2019 15:34

Changes have been addressed.

@anweiss
anweiss merged commit d46aa13 into docker-archive:master Jan 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants