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 Log Streaming URLs for project without a project name #140

Merged
merged 4 commits into from
Oct 21, 2021

Conversation

Aayyush
Copy link

@Aayyush Aayyush commented Oct 20, 2021

No description provided.

@Aayyush
Copy link
Author

Aayyush commented Oct 20, 2021

/ptal @nishkrishnan @msarvar @pcalley #orchestration

Copy link

@msarvar msarvar left a comment

Choose a reason for hiding this comment

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

There are several places that need to be updated to make this work.

AsyncProjectCommandOutputHandler uses ProjectCommandContext#PullInfo() to create a unique key for a project. Current implementation of PullInfo() only accounts for RepoName, PullNum, and ProjectName. We need to add additional Workspace to the PullInfo and use RepoRelDir as a ProjectName sub if ProjectName is blank.

From there you can unify the URL into 1: /jobs/{org}/{repo}/{pull}/{project}/{workspace}. Where {project} is either project name or directory.

And you will also need to update JobsController#newProjectInfo to parse workspace parameter from the URL.

Comment on lines 463 to 468
var projectIdentifier string
if projectName == "" {
projectIdentifier = strings.ReplaceAll(relDir, "/", "-")
} else {
projectIdentifier = projectName
}
Copy link

@msarvar msarvar Oct 21, 2021

Choose a reason for hiding this comment

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

let's move this to a helper function GetProjectIdentifier() for ProjectCommandContext and ProjectStatus. This way you don't have to duplicate the logic in two places. Since you are implementing the same function for two different objects, you can create a private function in the models package to be called by both objects

Copy link
Author

Choose a reason for hiding this comment

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

Done!

@Aayyush Aayyush merged commit 1321ab1 into release-v0.17.1-lyft.1 Oct 21, 2021
@Aayyush Aayyush deleted the aay/jobs-url branch October 21, 2021 23:28
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