Skip to content
This repository has been archived by the owner on Mar 1, 2022. It is now read-only.

Remove publish target from artman and config #473

Merged
merged 8 commits into from
Aug 3, 2018
Merged

Remove publish target from artman and config #473

merged 8 commits into from
Aug 3, 2018

Conversation

andreamlin
Copy link
Contributor

@andreamlin andreamlin commented Jul 25, 2018 •
edited
Loading

Remove (github and local) publishing artman configs, as these are not needed by ACTools or Yoshi team.

This reduces the size of the artman configs.

The old artman.tasks.publish.noop file is renamed as artman.tasks.emit_success, which more accurately describes that task.

In the CircleCI smoke tests, change all publish targets to generate.

Eventually, we would be able to remove the generate target because it's the only option, and it would be frivolous to specify it if there are no other options.

@andreamlin andreamlin added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jul 25, 2018
@andreamlin
Copy link
Contributor Author

Waiting for smoke tests to pass before sending out for review.

@andreamlin
Copy link
Contributor Author

PTAL

garrettjonesgoogle
garrettjonesgoogle reviewed Jul 25, 2018
View reviewed changes
Copy link
Member

@garrettjonesgoogle garrettjonesgoogle left a comment

Choose a reason for hiding this comment

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

This is awesome!

Before you submit it though, I was thinking we should do a couple other things before we yank this out:

  1. Delete all of the publishing config from the existing artman config files in googleapis
  2. Mark publishing deprecated in g3artman and have it print a failure if someone tries to use it

I just want to make sure there isn't an unanticipated use case before actually pulling the trigger.

@andreamlin andreamlin removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jul 25, 2018
@andreamlin
Copy link
Contributor Author

  1. Because googleapis is synced from internal repos, wouldn't the changes get overridden by internal pushes?

  2. Sure.

@garrettjonesgoogle
Copy link
Member

  1. Of course - the change would need to be done in the internal files (I thought that was implied...)

@andreamlin andreamlin changed the title remove publishing WIP Remove publish target from artman and config Jul 25, 2018
@andreamlin
Copy link
Contributor Author

  1. Any changes made to the internal files would get overridden as soon as an API producer regenerates or bootstraps a config using a version of artman without this PR.

@garrettjonesgoogle
Copy link
Member

  1. API producers won't regenerate artman files. In the case of config bootstraps, I think we can handle the 0-2 cases that might happen in the "gap" on a one-off basis.

@andreamlin
Copy link
Contributor Author

andreamlin commented Jul 25, 2018 •
edited
Loading

Ok, made an internal PR for removing all instances of the publish_targets.
Also made an internal PR for deprecating g3artman deprecating the publish feature in g3artman.

@garrettjonesgoogle
Copy link
Member

I think you mean a PR to deprecate the publish feature in g3artman?

@andreamlin
Copy link
Contributor Author

Ok, (1) and (2) are done!
PTAL

garrettjonesgoogle
garrettjonesgoogle reviewed Jul 30, 2018
View reviewed changes
artman/config/proto/config.proto Outdated
@@ -266,6 +252,7 @@ message Artifact {
// [dependencies.yaml](https://github.com/googleapis/googleapis/blob/master/gapic/packaging/dependencies.yaml)
PackageVersion package_version = 14;

// DEPRECATED.

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

artman/pipelines/code_generation.py Outdated
@@ -23,6 +23,7 @@
from artman.pipelines import pipeline_base
from artman.tasks import io_tasks
from taskflow.patterns import linear_flow
from artman.tasks import emit_success

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

artman/pipelines/gapic_generation.py Outdated
from artman import tasks
from artman.utils import task_utils


# kwargs required by GAPIC code gen
_GAPIC_REQUIRED = ['service_yaml', 'gapic_yaml', 'language', 'aspect', 'publish']
_GAPIC_REQUIRED = ['service_yaml', 'gapic_yaml', 'language', 'aspect',]

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

garrettjonesgoogle
garrettjonesgoogle approved these changes Aug 3, 2018
View reviewed changes
Copy link
Member

@garrettjonesgoogle garrettjonesgoogle left a comment

Choose a reason for hiding this comment

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

LGTM

@andreamlin andreamlin merged commit 539e324 into googleapis:master Aug 3, 2018
@andreamlin andreamlin deleted the remove_github branch August 3, 2018 19:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@garrettjonesgoogle garrettjonesgoogle garrettjonesgoogle approved these changes

@lukesneeringer lukesneeringer Awaiting requested review from lukesneeringer

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@andreamlin @garrettjonesgoogle