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 organization proto #589

Merged
merged 7 commits into from Nov 22, 2023
Merged

feat: add organization proto #589

merged 7 commits into from Nov 22, 2023

Conversation

masaaania
Copy link
Contributor

@masaaania masaaania commented Nov 22, 2023

This PR

  • adds functions and messages for the organization to proto.
  • implementation will be pushed in the next PR.

@masaaania masaaania marked this pull request as ready for review November 22, 2023 03:46
message UpdateOrganizationRequest {
string id = 1;
ChangeDescriptionOrganizationCommand change_description_command = 2;
RenameOrganizationCommand rename_command = 3;
Copy link
Member

Choose a reason for hiding this comment

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

I know a few places use the word rename, but what do you think about using change instead to fit the ChangeDescriptionOrganizationCommand for this service?

// See the License for the specific language governing permissions and
// limitations under the License.

package api
Copy link
Member

Choose a reason for hiding this comment

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

Just confirming. The ConvertTrialProject API is not implemented. Are you going to implement it later?

string description = 3; // optional
}

message CreateTrialOrganizationCommand {
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about adding a flag in the CreateOrganizationCommand to create a trial project to reduce the APIs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice idea 😄.

@masaaania
Copy link
Contributor Author

masaaania commented Nov 22, 2023

@cre8ivejp

I updated and fixed the PR. Please take a look when you have time.

string name = 1;
string url_code = 2;
string description = 3; // optional
string is_trial = 4;
Copy link
Member

Choose a reason for hiding this comment

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

Please use boolean instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cre8ivejp
Thank you, nice catch!! 🙏 🙏

Copy link
Member

@cre8ivejp cre8ivejp left a comment

Choose a reason for hiding this comment

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

Thank you!!

@masaaania masaaania merged commit 112f975 into main Nov 22, 2023
15 checks passed
@masaaania masaaania deleted the organization branch November 22, 2023 10:31
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.

None yet

2 participants