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

Verify that component namespace and name combinations are unique #685

Merged
merged 5 commits into from
Apr 24, 2024

Conversation

Grifs
Copy link
Collaborator

@Grifs Grifs commented Apr 22, 2024

Describe your changes

Verify that component namespace and name combinations are unique

Related issue(s)

Closes #684

Type of Change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • New functionality (non-breaking change which adds functionality)
  • Major change (non-breaking change which modifies existing functionality)
  • Minor change (non-breaking change which does not modify existing functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • This change requires a documentation update

Checklist

Requirements:

  • I have read the CONTRIBUTING doc.
  • I have performed a self-review of my code by checking the "Changed Files" tab.
  • My code follows the code style of this project.

Tests:

  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.

Documentation:

  • Proposed changes are described in the CHANGELOG.md.
  • I have updated the documentation accordingly.

Test Environment

@Grifs Grifs requested a review from rcannood April 22, 2024 12:09
// Verify that all configs except the disabled ones are unique
// Even configs disabled by the query should be unique as they might be picked up as a dependency
// Only allowed exception are configs where the status is set to disabled
val uniqueConfigs = allConfigs.groupBy(config => config.functionality.namespace.map(_ + "/").getOrElse("") + config.functionality.name)
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps you could also add organisation to this id. Wdyt?

Copy link
Collaborator Author

@Grifs Grifs Apr 23, 2024

Choose a reason for hiding this comment

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

But the organisation isn't used in the output path, so collisions could still happen then.
However, this reminds me that I wanted to look into using ViashNamespace.targetOutputPath(...) as the key generator - as that is what determines what would result in collisions and means that once we have more flexibility in the folder name strategy as previously discussed, we're halfway there in solving that too.

Wdyt? ping pong

Copy link
Member

Choose a reason for hiding this comment

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

I see -- good point.

Should we even consider the use-case where one might use multiple organisations in one project -- perhaps not?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The one I see most likely to happen is a project that sets the organisation but with one or more components where the organisation is not set. In that regard it's still 2 different values for the organisation.
In itself it doesn't really hurt much but it's probably an error.

If we want a check for that, I'd suggest displaying a warning and adding it to Viash 0.9 (or later).

@Grifs Grifs requested a review from rcannood April 23, 2024 16:07
Copy link
Member

@rcannood rcannood left a comment

Choose a reason for hiding this comment

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

LGTM!

@Grifs Grifs merged commit fea35e9 into develop_0_8 Apr 24, 2024
7 checks passed
@Grifs Grifs deleted the feature/check_component_name_uniqueness branch April 24, 2024 14:35
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.

2 participants