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

Support Multiple Schema Validations #1066

Draft
wants to merge 44 commits into
base: main
Choose a base branch
from

Conversation

samtholiya
Copy link
Collaborator

@samtholiya samtholiya commented Feb 14, 2025

Why Draft

  • I want to refactor the code to be more beautiful
  • add some trace and debug logs.
  • Adding test cases
  • The atmos schema i downloaded from our prod website is not updated. We need to udpate that as well
  • add a way to validate only partial yaml schema's based on keys provided by users
  • Find a better UX than the bellow.
    image

what

  • Added a new validator command to the CLI.
  • This command verifies the YAML structure of the atmos CLI.
  • f a schema key is present in the YAML, it validates the structure against the schema.

why

  • Ensures that configuration files follow the correct YAML structure.
  • Prevents misconfigurations and errors caused by invalid or malformed YAML.
  • Adds an extra layer of validation by enforcing schema compliance when applicable.
  • Improves reliability and usability of the CLI by catching issues early.

references

@mergify mergify bot added the triage Needs triage label Feb 14, 2025
Comment on lines 39 to 46
if len(validationErrors) == 0 {
u.LogInfo(fmt.Sprintf("No Validation Errors found in %v using schema %v", yamlSource, customSchema))
} else {
u.LogError(fmt.Errorf("Invalid YAML:"))
for _, err := range validationErrors {
u.LogError(fmt.Errorf("- %s\n", err))
}
os.Exit(1)
Copy link
Member

Choose a reason for hiding this comment

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

Nothing new should ever use u.Log* and instead should use the charmbracelet logger directly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

https://github.com/cloudposse/atmos/blob/main/pkg/utils/log_utils.go
We use charmbracelet logger even if we use from utils. So, this should be fine

Copy link
Member

Choose a reason for hiding this comment

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

No, we do that as a stop-gap measure until we update everything to use the charmbracelet logger methods directly.

All we can do with u.Log* is wrap errors, no semantic logging.

Please read up on semantic logging provided by charmbracelet and practice implementing it to understand the differences.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cool will check that. Thanks for the update

var ErrAtmosSchemaNotFound = errors.New("atmos schema not found")

var atmosData = map[string][]byte{
"atmos://schema": []byte(atmosSchema),
Copy link
Member

Choose a reason for hiding this comment

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

Please see the ticket this is not right

URL string
}

func (u *URLFetcher) Fetch() ([]byte, error) {
Copy link
Member

Choose a reason for hiding this comment

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

We should not write a new implementation. Everywhere else we use go-getter. At least we should be deliberate an out creating a new interface. We fetch URL's all over the place.

@osterman osterman changed the title add validate schema Support Multiple Schema Validations Feb 15, 2025
coderabbitai[bot]
coderabbitai bot previously approved these changes Feb 18, 2025
@mergify mergify bot removed the triage Needs triage label Feb 18, 2025
@mergify mergify bot added the triage Needs triage label Feb 19, 2025
samtholiya and others added 23 commits February 20, 2025 00:33
…estability-and' of https://github.com/cloudposse/atmos into feature/dev-3056-refactor-gogetter-utility-for-better-testability-and
…estability-and' of https://github.com/cloudposse/atmos into feature/dev-3056-refactor-gogetter-utility-for-better-testability-and
…estability-and' of https://github.com/cloudposse/atmos into feature/dev-3056-refactor-gogetter-utility-for-better-testability-and
…estability-and' of https://github.com/cloudposse/atmos into feature/dev-3056-refactor-gogetter-utility-for-better-testability-and
@mergify mergify bot removed the conflict This PR has conflicts label Feb 26, 2025
@samtholiya samtholiya force-pushed the feature/dev-3030-implement-schema-validation-for-vendoring-and-atmos branch 2 times, most recently from 4d3f699 to 248ad73 Compare February 27, 2025 09:05
@samtholiya samtholiya force-pushed the feature/dev-3030-implement-schema-validation-for-vendoring-and-atmos branch from 248ad73 to 8e2067a Compare February 27, 2025 09:08
}
}
if customSchema == "" && yamlSource == "atmos.yaml" {
customSchema = "atmos://schema"
Copy link
Member

Choose a reason for hiding this comment

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

Wrong URI. Atmos has many schemas, therefore we have to disambiguate them.

Comment on lines +8 to +9
//go:embed schema/atmos-manifest.json
var atmosSchema string
Copy link
Member

Choose a reason for hiding this comment

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

The issue here is there is an atmos stack schema, an atmos config schema, a vendor schema, a component schema, and a workflow schema.

This assumes we have one schema for Atmos, which we do not.

Copy link

[!WARNING] > #### This PR exceeds the recommended limit of 1,000 lines. > > Large PRs are difficult to review and may be rejected due to their size. > > Please verify that this PR does not address multiple issues. > Consider refactoring it into smaller, more focused PRs to facilitate a smoother review process.

Copy link

[!WARNING] > #### This PR exceeds the recommended limit of 1,000 lines. > > Large PRs are difficult to review and may be rejected due to their size. > > Please verify that this PR does not address multiple issues. > Consider refactoring it into smaller, more focused PRs to facilitate a smoother review process.

Copy link

[!WARNING] > #### This PR exceeds the recommended limit of 1,000 lines. > > Large PRs are difficult to review and may be rejected due to their size. > > Please verify that this PR does not address multiple issues. > Consider refactoring it into smaller, more focused PRs to facilitate a smoother review process.

Copy link

[!WARNING] > #### This PR exceeds the recommended limit of 1,000 lines. > > Large PRs are difficult to review and may be rejected due to their size. > > Please verify that this PR does not address multiple issues. > Consider refactoring it into smaller, more focused PRs to facilitate a smoother review process.

Copy link

mergify bot commented Feb 28, 2025

Warning

This PR exceeds the recommended limit of 1,000 lines.

Large PRs are difficult to review and may be rejected due to their size.

Please verify that this PR does not address multiple issues.
Consider refactoring it into smaller, more focused PRs to facilitate a smoother review process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/xl triage Needs triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants