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

feat: add --description flag to astro deploy command #1713

Merged
merged 17 commits into from
Sep 17, 2024
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ cover.out
coverage-2.txt
coverage.txt
dist/
hello-astro/
meta/
packages.txt
requirements.txt
Expand Down
8 changes: 7 additions & 1 deletion cmd/software/deploy.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ var (
DeployAirflowImage = deploy.Airflow
DagsOnlyDeploy = deploy.DagsOnlyDeploy
isDagOnlyDeploy bool
description string
)

var deployExample = `
Expand Down Expand Up @@ -56,6 +57,7 @@ func NewDeployCmd() *cobra.Command {
cmd.Flags().BoolVarP(&saveDeployConfig, "save", "s", false, "Save deployment in config for future deploys")
cmd.Flags().BoolVarP(&ignoreCacheDeploy, "no-cache", "", false, "Do not use cache when building container image")
cmd.Flags().StringVar(&workspaceID, "workspace-id", "", "workspace assigned to deployment")
cmd.Flags().StringVar(&description, "description", "", "Reason for the deploy update")

if !context.IsCloudContext() && houston.VerifyVersionMatch(houstonVersion, houston.VersionRestrictions{GTE: "0.34.0"}) {
cmd.Flags().BoolVarP(&isDagOnlyDeploy, "dags", "d", false, "Push only DAGs to your Deployment")
Expand Down Expand Up @@ -104,8 +106,12 @@ func deployAirflow(cmd *cobra.Command, args []string) error {
if isDagOnlyDeploy {
return DagsOnlyDeploy(houstonClient, appConfig, ws, deploymentID, config.WorkingPath, nil, true)
}

if description == "" {
description = utils.GetDefaultDeployDescription(cmd, args)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
description = utils.GetDefaultDeployDescription(cmd, args)
description = utils.GetDefaultDeployDescription(isDagOnlyDeploy)

nit: we could simply send the isDagOnlyDeploy instead of re-parsing/reading the value for the dags flag. Then in the GetDefaultDeployDescription function check if isDagOnlyDeploy != nil && *isDagOnlyDeploy == true

Copy link
Member Author

@Simpcyclassy Simpcyclassy Sep 13, 2024

Choose a reason for hiding this comment

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

I could also simply do if isDagOnlyDeploy right?

}
// Since we prompt the user to enter the deploymentID in come cases for DeployAirflowImage, reusing the same deploymentID for DagsOnlyDeploy
deploymentID, err = DeployAirflowImage(houstonClient, config.WorkingPath, deploymentID, ws, byoRegistryDomain, ignoreCacheDeploy, byoRegistryEnabled, forcePrompt)
deploymentID, err = DeployAirflowImage(houstonClient, config.WorkingPath, deploymentID, ws, byoRegistryDomain, ignoreCacheDeploy, byoRegistryEnabled, forcePrompt, description)
if err != nil {
return err
}
Expand Down
28 changes: 25 additions & 3 deletions cmd/software/deploy_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package software

import (
"fmt"

"github.com/astronomer/astro-cli/houston"
testUtil "github.com/astronomer/astro-cli/pkg/testing"
"github.com/astronomer/astro-cli/software/deploy"
Expand All @@ -25,7 +27,10 @@ func (s *Suite) TestDeploy() {
EnsureProjectDir = func(cmd *cobra.Command, args []string) error {
return nil
}
DeployAirflowImage = func(houstonClient houston.ClientInterface, path, deploymentID, wsID, byoRegistryDomain string, ignoreCacheDeploy, byoRegistryEnabled, prompt bool) (string, error) {
DeployAirflowImage = func(houstonClient houston.ClientInterface, path, deploymentID, wsID, byoRegistryDomain string, ignoreCacheDeploy, byoRegistryEnabled, prompt bool, description string) (string, error) {
if description == "" {
return deploymentID, fmt.Errorf("description should not be empty")
}
return deploymentID, nil
}

Expand All @@ -42,17 +47,34 @@ func (s *Suite) TestDeploy() {
err = execDeployCmd([]string{"test-deployment-id", "--save"}...)
s.NoError(err)

// Test when description is provided using the flag --description
err = execDeployCmd([]string{"test-deployment-id", "--description", "Initial deployment", "--force"}...)
s.NoError(err)

// Test when the default description is used
DeployAirflowImage = func(houstonClient houston.ClientInterface, path, deploymentID, wsID, byoRegistryDomain string, ignoreCacheDeploy, byoRegistryEnabled, prompt bool, description string) (string, error) {
expectedDesc := "Deployed via <astro deploy>"
if description != expectedDesc {
return deploymentID, fmt.Errorf("expected description to be '%s', but got '%s'", expectedDesc, description)
}
return deploymentID, nil
}

err = execDeployCmd([]string{"test-deployment-id", "--force"}...)
s.NoError(err)

// Restore DagsOnlyDeploy to default behavior
DagsOnlyDeploy = deploy.DagsOnlyDeploy

s.Run("error should be returned for astro deploy, if DeployAirflowImage throws error", func() {
DeployAirflowImage = func(houstonClient houston.ClientInterface, path, deploymentID, wsID, byoRegistryDomain string, ignoreCacheDeploy, byoRegistryEnabled, prompt bool) (string, error) {
DeployAirflowImage = func(houstonClient houston.ClientInterface, path, deploymentID, wsID, byoRegistryDomain string, ignoreCacheDeploy, byoRegistryEnabled, prompt bool, description string) (string, error) {
return deploymentID, deploy.ErrNoWorkspaceID
}

err := execDeployCmd([]string{"-f"}...)
s.ErrorIs(err, deploy.ErrNoWorkspaceID)

DeployAirflowImage = func(houstonClient houston.ClientInterface, path, deploymentID, wsID, byoRegistryDomain string, ignoreCacheDeploy, byoRegistryEnabled, prompt bool) (string, error) {
DeployAirflowImage = func(houstonClient houston.ClientInterface, path, deploymentID, wsID, byoRegistryDomain string, ignoreCacheDeploy, byoRegistryEnabled, prompt bool, description string) (string, error) {
return deploymentID, nil
}
})
Expand Down
9 changes: 9 additions & 0 deletions cmd/utils/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,12 @@ func EnsureProjectDir(cmd *cobra.Command, args []string) error {

return nil
}

func GetDefaultDeployDescription(cmd *cobra.Command, args []string) string {
// Check if the "--dags" flag was passed
if cmd.Flags().Changed("dags") {
return "Deployed via <astro deploy --dags>"
}

return "Deployed via <astro deploy>"
}
14 changes: 14 additions & 0 deletions cmd/utils/utils_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,3 +36,17 @@ func TestEnsureProjectDir(t *testing.T) {
err = EnsureProjectDir(&cobra.Command{}, []string{})
assert.NoError(t, err)
}

func TestGetDefaultDeployDescription(t *testing.T) {
// Test case where --dags flag is not set
cmd := &cobra.Command{}
description := GetDefaultDeployDescription(cmd, []string{})
assert.Equal(t, "Deployed via <astro deploy>", description)

// Test case where --dags flag is set
cmdWithDagsFlag := &cobra.Command{}
cmdWithDagsFlag.Flags().Bool("dags", true, "")
cmdWithDagsFlag.Flags().Set("dags", "true")
descriptionWithDags := GetDefaultDeployDescription(cmdWithDagsFlag, []string{})
assert.Equal(t, "Deployed via <astro deploy --dags>", descriptionWithDags)
}
4 changes: 2 additions & 2 deletions houston/deployment.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ var (
workspaceUuid: $workspaceId
releaseName: $releaseName
executor: $executor
airflowVersion: $airflowVersion
airflowVersion: $airflowVersion
namespace: $namespace
config: $config
cloudRole: $cloudRole
Expand Down Expand Up @@ -96,7 +96,7 @@ var (
workspaceUuid: $workspaceId
releaseName: $releaseName
executor: $executor
airflowVersion: $airflowVersion
airflowVersion: $airflowVersion
namespace: $namespace
config: $config
cloudRole: $cloudRole
Expand Down
10 changes: 7 additions & 3 deletions software/deploy/deploy.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ var tab = printutil.Table{
Header: []string{"#", "LABEL", "DEPLOYMENT NAME", "WORKSPACE", "DEPLOYMENT ID"},
}

func Airflow(houstonClient houston.ClientInterface, path, deploymentID, wsID, byoRegistryDomain string, ignoreCacheDeploy, byoRegistryEnabled, prompt bool) (string, error) {
func Airflow(houstonClient houston.ClientInterface, path, deploymentID, wsID, byoRegistryDomain string, ignoreCacheDeploy, byoRegistryEnabled, prompt bool, description string) (string, error) {
deploymentID, deployments, err := getDeploymentIDForCurrentCommand(houstonClient, wsID, deploymentID, prompt)
if err != nil {
return deploymentID, err
Expand Down Expand Up @@ -98,7 +98,7 @@ func Airflow(houstonClient houston.ClientInterface, path, deploymentID, wsID, by
fmt.Printf(houstonDeploymentPrompt, releaseName)

// Build the image to deploy
err = buildPushDockerImage(houstonClient, &c, deploymentInfo, releaseName, path, nextTag, cloudDomain, byoRegistryDomain, ignoreCacheDeploy, byoRegistryEnabled)
err = buildPushDockerImage(houstonClient, &c, deploymentInfo, releaseName, path, nextTag, cloudDomain, byoRegistryDomain, ignoreCacheDeploy, byoRegistryEnabled, description)
if err != nil {
return deploymentID, err
}
Expand All @@ -120,7 +120,7 @@ func deploymentExists(deploymentID string, deployments []houston.Deployment) boo
return false
}

func buildPushDockerImage(houstonClient houston.ClientInterface, c *config.Context, deploymentInfo *houston.Deployment, name, path, nextTag, cloudDomain, byoRegistryDomain string, ignoreCacheDeploy, byoRegistryEnabled bool) error {
func buildPushDockerImage(houstonClient houston.ClientInterface, c *config.Context, deploymentInfo *houston.Deployment, name, path, nextTag, cloudDomain, byoRegistryDomain string, ignoreCacheDeploy, byoRegistryEnabled bool, description string) error {
// Build our image
fmt.Println(imageBuildingPrompt)

Expand Down Expand Up @@ -170,6 +170,10 @@ func buildPushDockerImage(houstonClient houston.ClientInterface, c *config.Conte

imageHandler := imageHandlerInit(imageName)

if description != "" {
deployLabels = append(deployLabels, "io.astronomer.deploy.revision.description="+description)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: silly question, how does this description pop up in the UI when it's a DAG-only deploy because CLI is only setting it on the image, and I am not sure we do an image deploy when the --dags flag is passed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, should we extend this flag to --dag-only command too? @Simpcyclassy

Copy link
Member Author

Choose a reason for hiding this comment

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

Not a silly question at all. That is what this PR #1716 is for. It would send the description to dag server which creates a revision on houston

Copy link
Contributor

Choose a reason for hiding this comment

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

are we looking to release both the changes together?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes

Copy link
Contributor

Choose a reason for hiding this comment

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

why are we setting io.astronomer.deploy.revision.description

Copy link
Member Author

Choose a reason for hiding this comment

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

Because we are setting the description through the api request in the image

}

buildConfig := types.ImageBuildConfig{
Path: config.WorkingPath,
NoCache: ignoreCacheDeploy,
Expand Down
35 changes: 18 additions & 17 deletions software/deploy/deploy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
var (
errSomeContainerIssue = errors.New("some container issue")
errMockHouston = errors.New("some houston error")
description = "Deployed via <astro deploy>"

mockDeployment = &houston.Deployment{
ID: "cknz133ra49758zr9w34b87ua",
Expand Down Expand Up @@ -120,7 +121,7 @@ func (s *Suite) TestBuildPushDockerImageSuccessWithTagWarning() {
houstonMock.On("GetRuntimeReleases", "").Return(houston.RuntimeReleases{}, nil)
houstonMock.On("GetDeploymentConfig", nil).Return(mockedDeploymentConfig, nil)

err := buildPushDockerImage(houstonMock, &config.Context{}, mockDeployment, "test", "./testfiles/", "test", "test", "", false, false)
err := buildPushDockerImage(houstonMock, &config.Context{}, mockDeployment, "test", "./testfiles/", "test", "test", "", false, false, description)
s.NoError(err)
mockImageHandler.AssertExpectations(s.T())
houstonMock.AssertExpectations(s.T())
Expand Down Expand Up @@ -151,7 +152,7 @@ func (s *Suite) TestBuildPushDockerImageSuccessWithImageRepoWarning() {
houstonMock.On("GetDeploymentConfig", nil).Return(mockedDeploymentConfig, nil)
houstonMock.On("GetRuntimeReleases", "").Return(houston.RuntimeReleases{}, nil)

err := buildPushDockerImage(houstonMock, &config.Context{}, mockDeployment, "test", "./testfiles/", "test", "test", "", false, false)
err := buildPushDockerImage(houstonMock, &config.Context{}, mockDeployment, "test", "./testfiles/", "test", "test", "", false, false, description)
s.NoError(err)
mockImageHandler.AssertExpectations(s.T())
houstonMock.AssertExpectations(s.T())
Expand Down Expand Up @@ -183,7 +184,7 @@ func (s *Suite) TestBuildPushDockerImageSuccessWithBYORegistry() {
houstonMock.On("GetRuntimeReleases", "").Return(houston.RuntimeReleases{}, nil)
houstonMock.On("UpdateDeploymentImage", houston.UpdateDeploymentImageRequest{ReleaseName: "test", Image: "test.registry.io:test-test", AirflowVersion: "1.10.12", RuntimeVersion: ""}).Return(nil, nil)

err := buildPushDockerImage(houstonMock, &config.Context{}, mockDeployment, "test", "./testfiles/", "test", "test", "test.registry.io", false, true)
err := buildPushDockerImage(houstonMock, &config.Context{}, mockDeployment, "test", "./testfiles/", "test", "test", "test.registry.io", false, true, description)
s.NoError(err)
mockImageHandler.AssertExpectations(s.T())
houstonMock.AssertExpectations(s.T())
Expand All @@ -192,7 +193,7 @@ func (s *Suite) TestBuildPushDockerImageSuccessWithBYORegistry() {
func (s *Suite) TestBuildPushDockerImageFailure() {
// invalid dockerfile test
dockerfile = "Dockerfile.invalid"
err := buildPushDockerImage(nil, &config.Context{}, mockDeployment, "test", "./testfiles/", "test", "test", "", false, false)
err := buildPushDockerImage(nil, &config.Context{}, mockDeployment, "test", "./testfiles/", "test", "test", "", false, false, description)
s.EqualError(err, "failed to parse dockerfile: testfiles/Dockerfile.invalid: when using JSON array syntax, arrays must be comprised of strings only")
dockerfile = "Dockerfile"

Expand All @@ -208,7 +209,7 @@ func (s *Suite) TestBuildPushDockerImageFailure() {
houstonMock.On("GetDeploymentConfig", nil).Return(nil, errMockHouston).Once()
houstonMock.On("GetRuntimeReleases", "").Return(houston.RuntimeReleases{}, nil)
// houston GetDeploymentConfig call failure
err = buildPushDockerImage(houstonMock, &config.Context{}, mockDeployment, "test", "./testfiles/", "test", "test", "", false, false)
err = buildPushDockerImage(houstonMock, &config.Context{}, mockDeployment, "test", "./testfiles/", "test", "test", "", false, false, description)
s.Error(err, errMockHouston)

houstonMock.On("GetDeploymentConfig", nil).Return(mockedDeploymentConfig, nil).Twice()
Expand All @@ -220,7 +221,7 @@ func (s *Suite) TestBuildPushDockerImageFailure() {
}

// build error test case
err = buildPushDockerImage(houstonMock, &config.Context{}, mockDeployment, "test", "./testfiles/", "test", "test", "", false, false)
err = buildPushDockerImage(houstonMock, &config.Context{}, mockDeployment, "test", "./testfiles/", "test", "test", "", false, false, description)
s.Error(err, errSomeContainerIssue.Error())
mockImageHandler.AssertExpectations(s.T())

Expand All @@ -232,7 +233,7 @@ func (s *Suite) TestBuildPushDockerImageFailure() {
}

// push error test case
err = buildPushDockerImage(houstonMock, &config.Context{}, mockDeployment, "test", "./testfiles/", "test", "test", "", false, false)
err = buildPushDockerImage(houstonMock, &config.Context{}, mockDeployment, "test", "./testfiles/", "test", "test", "", false, false, description)
s.Error(err, errSomeContainerIssue.Error())
mockImageHandler.AssertExpectations(s.T())
houstonMock.AssertExpectations(s.T())
Expand Down Expand Up @@ -269,22 +270,22 @@ func (s *Suite) TestGetAirflowUILinkFailure() {

func (s *Suite) TestAirflowFailure() {
// No workspace ID test case
_, err := Airflow(nil, "", "", "", "", false, false, false)
_, err := Airflow(nil, "", "", "", "", false, false, false, description)
s.ErrorIs(err, ErrNoWorkspaceID)

// houston GetWorkspace failure case
houstonMock := new(houston_mocks.ClientInterface)
houstonMock.On("GetWorkspace", mock.Anything).Return(nil, errMockHouston).Once()

_, err = Airflow(houstonMock, "", "", "test-workspace-id", "", false, false, false)
_, err = Airflow(houstonMock, "", "", "test-workspace-id", "", false, false, false, description)
s.ErrorIs(err, errMockHouston)
houstonMock.AssertExpectations(s.T())

// houston ListDeployments failure case
houstonMock.On("GetWorkspace", mock.Anything).Return(&houston.Workspace{}, nil)
houstonMock.On("ListDeployments", mock.Anything).Return(nil, errMockHouston).Once()

_, err = Airflow(houstonMock, "", "", "test-workspace-id", "", false, false, false)
_, err = Airflow(houstonMock, "", "", "test-workspace-id", "", false, false, false, description)
s.ErrorIs(err, errMockHouston)
houstonMock.AssertExpectations(s.T())

Expand All @@ -298,35 +299,35 @@ func (s *Suite) TestAirflowFailure() {
// config GetCurrentContext failure case
config.ResetCurrentContext()

_, err = Airflow(houstonMock, "", "", "test-workspace-id", "", false, false, false)
_, err = Airflow(houstonMock, "", "", "test-workspace-id", "", false, false, false, description)
s.EqualError(err, "no context set, have you authenticated to Astro or Astronomer Software? Run astro login and try again")

context.Switch("localhost")

// Invalid deployment name case
_, err = Airflow(houstonMock, "", "test-deployment-id", "test-workspace-id", "", false, false, false)
_, err = Airflow(houstonMock, "", "test-deployment-id", "test-workspace-id", "", false, false, false, description)
s.ErrorIs(err, errInvalidDeploymentID)

// No deployment in the current workspace case
_, err = Airflow(houstonMock, "", "", "test-workspace-id", "", false, false, false)
_, err = Airflow(houstonMock, "", "", "test-workspace-id", "", false, false, false, description)
s.ErrorIs(err, errDeploymentNotFound)
houstonMock.AssertExpectations(s.T())

// Invalid deployment selection case
houstonMock.On("ListDeployments", mock.Anything).Return([]houston.Deployment{{ID: "test-deployment-id"}}, nil)
_, err = Airflow(houstonMock, "", "", "test-workspace-id", "", false, false, false)
_, err = Airflow(houstonMock, "", "", "test-workspace-id", "", false, false, false, description)
s.ErrorIs(err, errInvalidDeploymentSelected)

// return error When houston get deployment throws an error
houstonMock.On("ListDeployments", mock.Anything).Return([]houston.Deployment{{ID: "test-deployment-id"}}, nil)
houstonMock.On("GetDeployment", mock.Anything).Return(nil, errMockHouston).Once()
_, err = Airflow(houstonMock, "", "test-deployment-id", "test-workspace-id", "", false, false, false)
_, err = Airflow(houstonMock, "", "test-deployment-id", "test-workspace-id", "", false, false, false, description)
s.Equal(err.Error(), "failed to get deployment info: "+errMockHouston.Error())

// buildPushDockerImage failure case
houstonMock.On("GetDeployment", "test-deployment-id").Return(&houston.Deployment{}, nil)
dockerfile = "Dockerfile.invalid"
_, err = Airflow(houstonMock, "./testfiles/", "test-deployment-id", "test-workspace-id", "", false, false, false)
_, err = Airflow(houstonMock, "./testfiles/", "test-deployment-id", "test-workspace-id", "", false, false, false, description)
dockerfile = "Dockerfile"
s.Error(err)
s.Contains(err.Error(), "failed to parse dockerfile")
Expand Down Expand Up @@ -359,7 +360,7 @@ func (s *Suite) TestAirflowSuccess() {
houstonMock.On("GetDeployment", mock.Anything).Return(&houston.Deployment{}, nil).Once()
houstonMock.On("GetRuntimeReleases", "").Return(mockRuntimeReleases, nil)

_, err := Airflow(houstonMock, "./testfiles/", "test-deployment-id", "test-workspace-id", "", false, false, false)
_, err := Airflow(houstonMock, "./testfiles/", "test-deployment-id", "test-workspace-id", "", false, false, false, description)
s.NoError(err)
houstonMock.AssertExpectations(s.T())
}
Expand Down