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

add regex to get componentID from control mapping #51

Merged
merged 3 commits into from
Jan 25, 2019

Conversation

aitchkhan
Copy link
Contributor

add componentID from excel in generated implementation

impl/impl_test.go Show resolved Hide resolved
impl/implementation.go Show resolved Hide resolved
impl/implementation.go Show resolved Hide resolved
Copy link
Contributor

@anweiss anweiss left a comment

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
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
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
Copy link
Contributor

anweiss commented Jan 18, 2019

@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
Copy link
Contributor

anweiss commented Jan 23, 2019

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

@aitchkhan
Copy link
Contributor Author

aitchkhan commented Jan 23, 2019 via email

@mohuk
Copy link
Contributor

mohuk commented Jan 24, 2019

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
Copy link
Contributor

anweiss commented Jan 24, 2019

@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
Copy link
Contributor

anweiss commented Jan 24, 2019

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

Signed-off-by: aitchkhan <aitchkhan@gmail.com>
Signed-off-by: aitchkhan <aitchkhan@gmail.com>
@haroonKhan-10p
Copy link
Contributor

updated.

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

@anweiss anweiss left a comment

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.

None yet

6 participants