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

Feature/project description #8

Closed

Conversation

robert-ulbrich-mercedes-benz
Copy link
Collaborator

@robert-ulbrich-mercedes-benz robert-ulbrich-mercedes-benz commented Sep 13, 2024

Why are the changes needed?

This change allows to set the description of a Flyte project. It could hold the responsible team for a Flyte project including the contact data or some other information.

What changes were proposed in this pull request?

  • A new config map file was introduced including keys in the Helm chart were updated for the flyte-core Helm chart so they can hold a name and a description for the project
  • A new structure was introduced that holds the project information to be handed over to the database repository
  • For the flyte-binary and the single binary approach the structure of the Helm chart was changed

How was this patch tested?

  • The implementation was tested by running the regular set up process for the single binary approach
  • Also the regular flyteadmin migrate seed-projects was tested with the adapted flyte-single-binary-local.yaml config file

Setup process

Screenshots

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Related PRs

Docs link

The program was tested solely for our own use cases, which might differ from yours

Robert Ulbrich [email protected], Mercedes-Benz Tech Innovation GmbH, legal info/Impressum

Comment on lines 12 to 13
</component> No newline at end of file
</component>

Choose a reason for hiding this comment

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

Where does this change come from?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Automatically changed from IntelliJ

Comment on lines 75 to 80
projects := []config.Project{{Name: "flytesnacks", Description: "flytesnacks project"}}
if len(cfg.SeedProjects) > 0 {
for _, project := range cfg.SeedProjects {
projects = append(projects, config.Project{Name: project.Name, Description: project.Description})
}

Choose a reason for hiding this comment

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

AFAIK it will keep "flytesnacks" even if someone defines his/her own projects, right? Previoulsy it wasn't the case

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are right, I have fixed it

Copy link

@LucasZanellaMBTI LucasZanellaMBTI left a comment

Choose a reason for hiding this comment

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

Good job!

Please consider simplifying your feature branch by dropping merge commits.

@@ -73,7 +73,7 @@ func Test_tokenEndpoint(t *testing.T) {

m := map[string]interface{}{}
assert.NoError(t, json.Unmarshal(rw.Body.Bytes(), &m))
assert.Equal(t, 0.5*time.Hour.Seconds()-1, m["expires_in"])
assert.GreaterOrEqual(t, m["expires_in"], 0.5*time.Hour.Seconds()-5)

Choose a reason for hiding this comment

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

Personally I would separate this using a dedicated fix but not sure if flyte guys care about

@LucasZanellaMBTI
Copy link

@robert-ulbrich-mercedes-benz please could you get rid of merge commits and squash changes into a single commit

@robert-ulbrich-mercedes-benz
Copy link
Collaborator Author

Good job!

Please consider simplifying your feature branch by dropping merge commits.

I rebased the commits

# Conflicts:
#	docker/sandbox-bundled/manifests/complete-agent.yaml
#	docker/sandbox-bundled/manifests/complete.yaml
#	docker/sandbox-bundled/manifests/dev.yaml
#	flyteadmin/pkg/manager/impl/testutils/mock_requests.go
#	flyteadmin/pkg/manager/impl/validation/execution_validator_test.go
#	flytepropeller/pkg/compiler/validators/bindings_test.go
@LucasZanellaMBTI
Copy link

Please add required footer sections to PR description as outlined here

@robert-ulbrich-mercedes-benz
Copy link
Collaborator Author

Please add required footer sections to PR description as outlined here

What exactly do you mean? I will close this pull request. I will not further follow it, but open a pull request against flyte.

@LucasZanellaMBTI
Copy link

Completely aware of it. Nevertheless I'd prefer to have as much as possible being reviewed upfront. At least for me, legal notices are worth to be reviewed as well.

@robert-ulbrich-mercedes-benz
Copy link
Collaborator Author

Completely aware of it. Nevertheless I'd prefer to have as much as possible being reviewed upfront. At least for me, legal notices are worth to be reviewed as well.

I added the the legal notices.

@LucasZanellaMBTI
Copy link

Completely aware of it. Nevertheless I'd prefer to have as much as possible being reviewed upfront. At least for me, legal notices are worth to be reviewed as well.

I added the the legal notices.

I think in PR desc follwing phrase is misssing:
The program was tested solely for our own use cases, which might differ from yours

@robert-ulbrich-mercedes-benz
Copy link
Collaborator Author

Completely aware of it. Nevertheless I'd prefer to have as much as possible being reviewed upfront. At least for me, legal notices are worth to be reviewed as well.

I added the the legal notices.

I think in PR desc follwing phrase is misssing: The program was tested solely for our own use cases, which might differ from yours

I adde the phrase.

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